tP comments dashboard

Sorted based on the number of comments given to others' PRs, but also showing comments on own PRs and other comments given. Updated weekly.

[This page was last updated on Mar 25 2021]

Should we use the actual website links instead of the link to the markdown file?

Should there be a corresponding w-yuchen.png image uploaded as well?

Should there be 2 formats for the sort command as well?

Perhaps this should be updated too

Should this be left out because the use cases are supposed to show only what the user can see?

Similar to the previous comment

I guess this is down to the implementation already.

Will you want to

execute the edit command even if there are erroneous fields or
abort the whole command entirely and show an error?

The add command follows number 2 for now.

Good point raised, we can follow up on this discussion about which format we want with the other team members.

I think this may have been left out by mistake

Rmb to update this javadoc

I think this may be removed as it is handled above

All the EditPropertyCommand.EditPropertyDescriptor can be simplified to just EditPropertyDescriptor since EditPropertyDescriptor is an inner class of EditPropertyCommand

Perhaps this should be removed

Remember to update all the javadocs too

Maybe can do a import seedu.address.logic.commands.EditPropertyCommand.EditPropertyDescriptor; on top so that you can directly use EditPropertyDescriptor instead of the whole verbose type

Rmb to change this javadoc

Rmb to update this variable name

Rmb to update this javadoc

Should the message_usage be from FindPropertyCommand instead?

Rmb to update this javadoc

Should this description should include the appointments also?

Perhaps this part should be under the switch clause instead

Should this two be getTypicalAppointmentBook() instead?

Should this two be getTypicalPropertytBook() instead?

Should this two be getTypicalAppointmentBook() instead?

Should this two be getTypicalPropertyBook() instead?

Ok thanks

I think it is?

Should this two be getTypicalAppointmentBook() instead?

Should this be getFilteredAppointmentList() instead?

Don't change this because the time is supposed to be displayed as "9:00AM" and not "9:00am"

Rmb to update this to getTypicalPropertyBook()

Can simplify to just SortAppointmentDescriptor instead of SortAppointmentCommand.SortAppointmentDescriptor

Same goes for other instances in this whole file

I think this should be sorted in %1$ending order instead right? Because its ascending and descending.

Will this throw an error if user did not pass in either asc or desc in the command? Is the asc/desc supposed to be optional instead?

Think there is a typo here. This whole file is using EditPersonDescriptor instead of SortAppointmentDescriptor.

But I think for SortAppointmentTest, the testing should be of a different format too right?

Oops right!! My bad.

Ohh, ok sure

Remember to update the UG in this case

Ok, do rmb to update it in future

Rmb to update this javadoc

Any reason for this change?

I think it will be neater to create an empty constructor in Client class.

There will be an extra ; in the toString of Property when both the client information and tags are printed

E.g. Client Asking Price: $800,00;; Tags: [4 bedrooms]

Rmb to update this javadoc

Is there a reason for using .withName(VALID_NAME_BURGHLEY_DRIVE)?

Just a suggestion: Would you like to consider using the ArgumentTokenizer class here?

Perhaps this can be changed to:

"Parameters: [KEYWORD]... [pl/UPPER_PRICE_LIMIT] [pm/LOWER_PRICE_LIMIT]"

instead and put the note on inclusive in the description? So that it aligns with our command format of replacing the uppercase characters in the command format with user input.

Rmb to update this to property book

I think it may be better to standardize your prefixes by adding a space behind "proceed" and "cancel" as as well

Have you considered the use of forward slashes in these new prefixes? Such as new/, proceed/ and cancel/

What may be some of the reasons you choose not to use them?

Rmb to remove this extra line

Maybe this description can be more descriptive?

Rmb to update this javadoc

Is there a reason for this change? Have you tested with resizing the app? Because I am not sure if this will affect when the user resize the app (e.g. maximize it)

Would it be better if you leave all these changes to the default one? Because I think it is the conventional way to express as top, right, bottom, left.

Similarly for this, I think there is no reason to change their positions?

Good that you make it consistent with the others

Is there supposed to be a use for this? I can't find any place that uses this matcher object

Maybe you will like to include an example usage as well

Will you like to move this to the UpdateCommand parent class instead so that it is not duplicated in UpdateProceedCommand class?

If you are moving the MESSAGE_NULL_STATUS to UpdateCommand class, then perhaps this should be moved as well for consistency

Same as the comment in UpdateCancelCommand

It seems weird to me that next() for Completion returns a Completion?

Ohh, I see

Would it be better if the .next() method is never called for Completion? But I do understand that it has to implement the method in this scenario. So I guess its fine.

But perhaps one other suggestion is to show the user that it is already completed or something if user pass in a command to update a completed property to the next stage?

Yeapp!!

Didn't realize that the update command is called as such update 1 proceed/, without anything behind the prefixes. So they are not really prefixes in this case, so maybe the forward slashes are not intuitive for the user and should be removed. Sorry!!

An alternative suggestion may be to change the command format to update INDEX s/STATUS?

E.g. update 1 s/NEW, update 1 s/proceed, update 1 s/cancel

In this case, you can even make use of the ArgumentTokenizer and ArgumentMultiMap classes to parse the inputs.

Will this be better?

Maybe change Undoes to Undos?

Undid?

Undos?

Correct me if I am wrong but do you have to set the previousAppointmentLists of previousAppointmentBook as well? So that the user can call undo two times in a row?

Same as the comment for AppointmentBook

Can this method be shifted to PocketEstateParser instead? So that the regex and parsing are not duplicated.

Let me know if it is not possible because it will affect the implementation or if there are any other reasons

Actually, the main question is just whether the user can call undo two times in a row now?

Ohh,that's good! Then we can merge this.

I think we shouldn't add this kind of setting files to the team repo. Can you leave this file out or put it in the .gitignore?

I don't think we should change this file too

I think we should be consistent with the add property command and use [t/TYPE] to indicate that the filter is for property type

Do you mean accepted here?

Can reuse the PREFIX_TYPE defined earlier in the file instead of defining a duplicate one

Is there a reason for defining a separate method to get properties with clients? Will it be better to just include JURONG in the getTypicalProperties method?

What does julLogger stand for? Will it be better to use a more descriptive name?

I think we should be consistent and use t/PROPERTY_TYPE here

Need to remove appointments here

This should be appointments instead

I think it will be better if the ... are after the square brackets.

E.g. find property [KEYWORD] [OPTION]...

withAppointment takes in an actual Appointment object. So if an example is given, it will be quite verbose.

E.g. AppointmentBook ab = new AppointmentBookBuilder().withAppointment(new Appointment(new Name("Meet Alex"), new Remark("remark"), new Date(LocalDate.parse("2021-01-01")), new Time(LocalTime.parse("20:00:00"))).build();

So I don't think it is necessary to put. Let me know if yall think we should include it.

Ohh right good point. Shall change it. Thanks.

Thanks, merged and updated this branch

Update target user profile and value proposition.

Add glossary.

Add user stories.

should I close this pull request?

No, this is for the tutorial only.

Do remember to update the command summary for these commands at the bottom of the user guide page as well

Only property types "hdb", "condo" and "landed" and all forms of their capitalization are allowed for the PropertyType attribute of a property

I think that we originally discussed and decided that the asking price should be an optional "client asking price", so I had put it under client info. We can discuss more about this in the next meeting to see if we want to change it to be under property directly.

Agreed. Full name for everyone would be great.

Agreed

@Eriksen2411 Yes 😃

In that case would it be

In charge of: Deliverables and deadlines

Role: Deliverables and deadlines

Role: In charge of Deliverables and Deadlines

I think there should be a space between // and Set.

I think there should be a space between // and Maximise.

Nice touch.

Add space below.

Yup, I'll add an issue!

Nice

colab_icon_32 would be easier to differentiate. 32 being an example size.

Perhaps all colour constants can have a prefix. COLOUR_PRIMARY_LIGHT and COLOUR_ACCENT.

COLOUR_CELL would be better. "Cell" is unclear.

What's the reason why it is NaN?

Nice!

Any reason for this specific resolution? Perhaps a generic 1280x720 or 1920x1080 would be consistent among different computers.

line space between if statement and previous line

line space

indentation. && should be aligned to ||.

Do add the types in the array list as well. It's failing the checks.

"new" has been duplicated twice.

Here

A list of Persons -> Tasks instead.

This one can use requireNonNull(), but no big deal.

I love how your comments are so detailed btw! 😃

Should it be target.isSameProject(edited) && contains(editedProject) without the ! in front of target.isSameProject(edited)?

Nothing implemented here

Should there be a requireNonNull(project) here?

Should there be a requireNonNull(key) here?

Need to implement. Why return null?

Should the predicate being imported be PROJECTS instead?

Here too.

Personally I feel that if we have to reference something from the Command class, then it might be better to put this message within the Command class rather than Messages. What do you think?

I like this change, but maybe wanna add the requireNonNull(projectToEdit); before this line.

Not a big deal but I think having the comma makes sense.

Why not have 2 constructors. The one with 3 parameters shall require non null for all 3 fields. So that when it is used externally, it is used correctly. The one with 2 parameters shall require non null for 2 fields.

This will then need to be updated.

Lets standardise the way this message_usage String is formatted in the future 😃

@samuelfangjw Referring to standardising it over all the command classes. Not too important for now. I wanted to neaten it for the other command classes.

Was referring to this part, that refers to the COMMAND_WORD. Not a big deal for now since command word will most likely be unchanged.

@samuelfangjw Added a new comment to the correct line for this.

Sure!

Add extra line here

Extra line here.

Add extra line here.

Just a minor suggestion. I think convertedProjectName would be clearer here. Since there are many variables starting with "project" and "projectName"

Is there a reason why you do this once here, and then assertDoesNotThrow for the same thing below?

Looks repeated as the one below.

Looks repeated as the one here.

Can the Set(i,deadline) method be used instead. This way the list would not get reordered unnecessarily.

Do you wanna make this change to EventList too?

Do you wanna simplify "deadline.getIsDone() ? "✔" : """ with a method in Deadline? Or even from toString?

Do you wanna simplify "todo.getIsDone() ? "✔" : """ with a method in Todo? Or even from toString?

I think it would be better if DP can be labelled better. Like DP_COLOR_NO_TRANSPARENCY etc

Same as above

Got it, then DP_1_ELEVATION would be good

Sounds good to me!

I think it is better to have it in the previous form, so that the ParseException e's message can be shown to the user as well.

Although the new design does simplify things.

The method header name here is wrong. For other methods, your method header names also seem irrelevant to the what the test actually does.

Wrong method header.

Header names here are fine

Oh okay. In that case, nice catch 😃

Good idea!

So for now there is not mechanism to tick the todo off in via the GUI right? It'll just update the GUI accordingly?

Was id left out intentionally?

Is the ID here intentionally left out?

id here too

Reading this, it looks like ID should be checked too. Hmm

here too.

Maybe the UiCommand can be specified to be ViewProjectUiCommand since that is the current context?

and here.

Looks better man!

Is it overridden instead of overwritten?

Looks better man!

Got it man, thanks

Perhaps can add requireNonNull for dateOfEvent

requireNonNull

Looks like id isn't being used at all.

I remember the implementation here was changed, or is this correct?

Perhaps can requireNonNull here for Event just to be thorough

Wanna change the string text for this and deadlines to a constant declared at the top for easy access?

In reference to the previous PR haha

I see. That does make sense and simplifies a lot of things!

"events list" can be changed to "EventList"

issues & ececutes change to "executes"

It checks Event provided is valid or not -> It checks if Event provided is valid or not

"an exception will be thrown, Ui will" change to "an exception will be thrown and Ui will"

It check whether -> It checks whether

and Event provided is duplicated or not -> and if Event provided is duplicated.

I kind of don't understand what is being said here.

works -> work

works -> works.

remove the word "issues".

-> The user executes the command

not work with the immutable -> not work with an immutable

Oh nice catch. thanks @Eriksen2411 !

np!

Good catch. Added it. Thanks!

Yup! I took from address field because that allows for long and varied sentences. I think the one from project name allows only for 2 words

Thanks!

Thank you. I will merge the changes and request review again 😃

Will resolve this at a later date. Placed in issues. 😃

Good idea!

Thanks!

Nice thanks!

Nice. fixed!

LGTM

Dimensions have been updated.

@samuelfangjw thanks. I have updated the title and the description.

Pull Request was closed due to deletion of branch. No merging was done.

@samuelfangjw Looks great! Please add the changes you have added to features to the table of contents.

Nice work! LGTM!

LGTM.

Nice work. LGTM!

@samuelfangjw the images that show the features in detail. It can be seen in the pages further below in the user guide.

@samuelfangjw Good catch 😂

@samuelfangjw I think you've found the one 😂

Are you able bring back the v1.1 milestone?

@samuelfangjw Looking forward to it hahaha!

@samuelfangjw Nice thanks. I'll get on it soon!

@samuelfangjw Good idea!

Closing as Tutorial PR irrelevant to project.

Thanks! @samuelfangjw

Nice. LGTM man!

Might need to check the things brought up in the comments. Accidentally approved and left out this comment.

All the models, storage and tests have been updated. PR is ready for review. Thanks!

Tests aren't sufficient, but we'll ignore that for now so that the rest can proceed. I'll add more tests over time.

Done up all the necessary classes for AddTodoCommand and related classes. Some changes and additional checks were added/made to AddEventCommand and related classes. I have yet to do up tests for AddTodoCommand. I will do AddDeadLineCommand after this, and then will add the tests for Todo and Deadline at the same time. Thanks!

Done up all the necessary changes for AddDeadlineCommand and related classes. Some changes were made to AddTodoCommand and AddEventCommand. I will work on deleteE/T/D and then do the tests for all. Thanks!

Ready for review. Tests will be added at a later date after deleteEvent & deleteDeadline are done.

Ready for review. Will add tests soon after merge.

Ready for review.

Done up the tests for AddTodoCommand and it's related tests.

Ready for review! Thanks!

Made relevant changes and fixed some issues. Fixed a minor issue with a test for DeleteContactFromCommandTest. Ready for review. Thanks!

Ready for review! Thanks!

Ready for review, thanks!

This might sound like a bit of nitpicking, but then if I don't other teams might, the "adds" in this sentence can be confusing, perhaps we can use something like append/include/insert? Same for the things below.

I think this is the same issue as what you have raised about separation of the add/delete of dog/owner, in this case shall we just keep things simple and simply call it "missing required information"? To maintain clarify we can put a note at the bottom saying what field are required for dogs and owners. Doing it this way reduces clutter since we can combine more things together, makes any future edits less likely to miss out any detail.

the image name needs to be wei-yutong.png to match your github username, you should be able to rename it easily thru gui or by using the git mv command

Your link is https://nus-cs2103-ay2021s2.github.io/tp-dashboard/?search=&sort=groupTitle&sortWithin=title&since=2021-02-19&timeframe=commit&mergegroup=&groupSelect=groupByRepos&breakdown=false&tabOpen=true&tabType=authorship&tabAuthor=wei-yutong&tabRepo=AY2021S2-CS2103T-T10-1%2Ftp[master]&authorshipIsMergeGroup=false&authorshipFileTypes=

the blank lines prevent the table from appearing correctly

Remove this line perhaps?

Maybe we can include dogs and programs in here too?

I feel that this line a bit weird? It does make sense though if we have the feature to take note if someone has paid for the upcoming program.

There's the black circle symbol, is this intended?

For headings I think it is fine to leave out the bold if we are going to use headings. It is also necessary to leave a space in between the # and the word after for it to take effect.

As in the hash-tag is a hash-tag not used as heading? Because throughout the document it seems like they are intended to be headings, if you preview the rendered markdown.

Yea, you can refer to our submitted user guide.

Oh but if we still want to keep the dot right, would it be better to use the markdown representation so it is rendered nicely?

I closed it because idk how to rename the commit message xD.

You can try git commit --amend -m "Your new commit message".

this might need to be a part of the addressbook instead, given that storage will likely modify it after reading back the saved file, and especially since in the tests there will be different addressbooks created, but a simple equal will fail since there is no way to sync up the id numbers

isn't it just otherOwner.getID() == getID()? and I think we can do away with this check since it is reasonable enough that 2 owners with all details that are the same point to the same person, and some tests also uses this method to compare between owners.

maybe do some basic checks at this point for the owner keyword? the tests can also be updated this way and at least the syntax will be updated.

This is a really strange change, might be due to the problematic git history.

This is not a bug, if you look carefully at the condition that triggers the exception, it is either when some compulsory arguments are missing or that there is some additional things in between the command keywords and the arguments. In this case, saying the unknown argument might be applicable, but only for the latter scenario. The more useful thing choice is to display help for the particular command or even subcommand like add owner will display the specifics are adding an owner.

Try not to introduce additional line breaks that do not have much meaning.

I like that you have added in the javadocs, however, this is to be kept private as it is an internal helper method that should not be exposed to any other users. Perhaps you made it public in order to write test code for it, but I think it is better to refrain from that since it is the public API that we are testing out, the test code should not care about the actual implementation so long as it does what it promises. Else we will fall into this never ending issue of making all methods of every class public. I could have inlined these two lines into the switch case of the main parse method, but for SLAP I created the helper functions.

As commented above, this testing should not happen anymore, but should instead be tested through the parse method. Also, try to make use of the constants already defined to come up with the command instead of the magic literals.

The brackets of the the lambda arguments should not be split into 2 lines, especially not when they are empty.

checkstyle has passed for these code or else it won't have been possible to merge them, so I think it might be a problem with your IDE configuration.

Yes, you can take a look at the rest of the code base and see how it is done for the others. Especially for helper methods that are more concerned with internal implementation, they could be very dynamic and change every time.

is this going to match all kinds of date formats or just our specified one?

I think we came to a conclusion last week that this field is to be storing an ID of the owner.

does it mean dogs can have the entire spectrum of sexuality now?

Nope this is perfect. But just need to make sure that the LocalDate parser is able to handle this variety. Maybe add a comment above it to indicate what it is looking out for? It will help make it more readable.

Yup, the idea here is to take in the ID and verify with the backend that the owner does exist for this ID to be considered valid. But you can just put some placeholder functions now when it comes to the adding part.

Hmm, then do we want a validation regex just like the date?

a bit of nitpick but got extra lines

perhaps there's a more succinct way to express this, but i think it's fine for now

I was wondering if it is better to store the date of birth and calculate the age from there.

Oops

I'm not so sure if we want interface templates instead of abstract base class, since interfaces only define methods, no common data like name or tag can be made common, the other thing is when it comes to writing functions like find it might be difficult to avoid boilerplate code.

where the spectrum hahaha

Maybe we can avoid the dilemma just by calling it Sex, since gender is what you identify with, sex is biological, easier to just have an other instead.

Is it better to store it as LocalDate instead? this will simplify a lot of things especially for the getAge method. And it will be easier for us to manipulate into any other format elsewhere, say the UI and json.

Is it cleaner if we just parse using LocalDate right from the start? Something like LocalDate localDate = LocalDate.parse(dob, DateTimeFormatter.ofPattern("d-M-yyyy"));

Is the indentation here supposed to be 8 spaces? Can consider updating your IDE settings to automatically indent by 8 spaces.

package private?

Tbh I'm quite keen to remove the use of INDEX__, because we have switched to referencing by ID and not by the visible index number. For this method it is fine of course, but there is a need to probably remove Index and create an ID class in its place, but that's probably another issue to be created.

Not to mention the convoluted construction and usage of the Index class doesn't seem to help.

I only have myself to blame for this, maybe at least we can extract it into some form of a helper method in the util class? it isn't very straightforward that we are deleting the first dog since we are still using getTypicalAddressBook just from a different class.

This is referring to the ModelStub btw

As in because previously there was only one CommandTest so given ModelStub's nature it made sense to just have it as a private class of the Command, but going forward we probably want to abstract out a bit of these test code as well. The ModelStub should become a package private shared by all the CommandTests, it will be good to combine the tests under AddCommandTest.

do you want to change it to entity-level

int can never be null, that's why it doesn't have the check. and perhaps you can update the javadoc to refer to Entity while you're at it?

can just delete directly since all usage already changed

hmm why expose the this actually?

I think can revert this portion of the commit and preserve the one above.

I don't quite understand the reason behind this change. There's another one below as well.

This particular overload is to make it possible to use the same ID across restarts using the one stored in the file. The check for ID is not redundant since it is possible that someone modified the file to make 2 entities have same ID, then they will come and file a bug.

same as what was mentioned above.

this is very dangerous, it exposes the internalList directly to modifications beyond this class, is there a reason for this?

maybe use a valid ID number? I think I have settled on positive integers.

okay I think i get the rationale for the returning the internalList directly, but it is alright to use the unmodifiable version as many other tests does the same. Also I think if this is targeted at owners but will not test dogs, it is a good idea to filter using instanceof as well.

same the other one

Okay I understand this portion now, it might seem fine to do this for now, but the issue is that this codebase isn't very small, having equivalent api will confuse others that are not so familiar with this portion, and any direct modification is now possible since Java passes by reference, it will be difficult to track down bugs if someone modifies it and violates the assumptions we have about unique entities etc.

this can be another test to remove an invalid ID equivalent to the null test above.

this indentation doesn't make sense, since it is a continuation of the bracket of the previous line, the && on this line is not at the same level as the || on the previous line.

Could use some updated variable names.

This also can be renamed.

Here too!

Naming for this as well.

extra line breaks!

is it better to use the hasEntity method to check if the ID exists and throw an exception if it doesn't. then we can assume that getFilteredEntityList will return something that definitely contains the owner, then we retrieve it for real this time and check if it is indeed an owner object instead of any other entities. It will be better to use the setEntity method to change the target owner by creating a new copy with the up to date details.

arrayset might be better?

as mentioned in the AddDogCommand portion, it will not necessary to directly modify the Entity objects since all attributes are constant throughout the lifetime of the object. This is to align to the original AB3 codebase whereby the edit command also modifies by creating an updated instance of Person before asking the model to set it as the new one.

there shouldn't be a need to do this as well.

Hmm then maybe HashSet will do also, btw can just leave it without the specific type of implementation, i.e. List instead of ArrayList, Set instead of Hashset, there's a discussion on the forum on this.

and a set makes more sense for our case, since we don't really need an order to them, it is more ideal in the sense, but why not get it right at the start and not worry later.

Another thing of note is that we have to ensure that the check that the entity we have is an instance of Owner first, or else we might be downcasting illegally, it would be a good opportunity to throw an exception along the lines of id doesn't refer to owner.

Will it be better if we keep naming a little more consistent here? Since elsewhere in the I have already used something like idEntityPair, it will be confusing to see idEntity here since it can be ambiguous. I think a better alternative would be to simply name it idNumber or simply just id.

There's an extra line break here. Other than that, another naming thing again, this applies to other areas as well, is it better if we use either id or ID in the naming and don't mix the casing? It helps to make things consistent as well.

Naming is especially confusing for this part, since idEntity supposedly refers to a pair already.

Refrain from commenting like that, a better practice when using git is to simply delete all these lines that you don't need or want to disable, commit this set of changes as an individual commit. If you intend to fix it later, maybe leave a todo around the original area. This way even if we don't implement it there won't be a need to clean up the code much. But if you do intend to restore this portion, it will be simply a matter of finding the commit that removed the lines and reverting it, creating an additional commit that essentially negates the removal yet retaining full history.

this makes Owner mutable.

and also this.

Maybe can declare an integer on top first but don't initialise it? then in the try part can use the result of parseInt such that it can be directly returned below.

indent one more layer.

This is not something that is meant for an assertion, since it is one of the expected possible errors that we need to handle. Assertions are for verifying assumption that should not be broken and if they are indeed broken, the program is unable to handle and we need to investigate further. I suggest that this be removed.

this needs to be removed

this instance here is redundant as well, since it will cause the if below to trip otherwise.

is it better to separate into 2 conditions so the error message can be a lot more precise?

instead of calling getEntity multiple times, it seems better if the result is saved first and reused later, the code will become more readable this way. and i think it is fine to separate the 2 checks here as well, since for a lot of other commands it fails at the first incorrect one.

You might want to go to Settings > Editor > General > On save, and enable Ensure every saved file ends with a line break, can prevent future CI from failing.

that EOL thing is really quite annoying, can refer to anli's pull request where I detailed the instructions to make the ide add the extra line at the end of every file

do you want to split this PR into 2 though? like it is easier for someone else to just merge your README updates separately, plus adding the mockup image first also allow everyone to get a copy from master faster

Quick question, do you intend to do the formatting now or just leaving it in this state? Because as you are the first to completely rewrite the user guide, this will set the precedence for others.

Also, you might want to create an issue and assign it to yourself for this.

You need to rename the image itself too!

Duplicate of #51

You might want to install the checkstyle plugin on intellij and run it locally before committing. Another way is to run them using the command line via gradle like what the course material mentioned, which I will not repeat here. There are two advantages to this, firstly you get feedback faster, since it is much quicker to run locally than for the CI to return its results, next several of us are watching this repo, each pull request and separate commits pushed to a PR will generate notifications/email for everyone subscribed, reducing unnecessary notifications will be helpful for everyone.

Another thing I've noticed is that intellij files are seeping in, i.e. src/.idea, by right .gitignore is configured to not add these files, you might want to check your setup to ensure that the excluded files are indeed excluded, else your future PR might still have the same issues.

Merged #110 instead.

Kindly sync your master with the team repo and rebase this branch on top of the latest master, dropping the merge commits along the way as well. The current state of the PR has both unnecessary and also wrong changes.

If you look at the commits you're trying to merge, there's 5, only the last 2 are really relevant. The merge commits when you merge master into this branch can very easily clutter up the commit history, making it hard to find bugs later on. If you do not want to rebase, try to avoid merging unless you need to resolve conflicts. Otherwise, a rebase is a good idea to keep the git history linear. I will elaborate on the problematic code in the review.

you need to fix your git history, you're recommitting past commits and changing the git history. it will cause trouble for the reposense tracking

@branzuelajohn after merging this you should be able to rebase your branch on top of it and be able to merge #137

I think this phrasing would be better.

in a university to keep track of people they have crossed paths with. It is optimised for use via...

I think putting my full name here will be better! (consistent with the other names)

Fang Junwei, Samuel

perhaps we can change the phrasing of the roles for all of us to In charge of .... e.g. In charge of documentation, In charge of Integration. Currently the role sections sound a bit weird for all of us.

Perhaps Tests instead of test would be better here.

typo in MIT Licence

We should consider ordering the glossary alphabetically. I think that was the initial ordering.

Missing period.

I think this is fine for v1.2 but let's add an issue to add support for different date formats, especially since output format and input format are different.

Is there a need for taskType?

Seems like a hacky way to keep track of subclass name and might not be best practice. We should instead be looking to utilise polymorphism to distinguish the different classes. If absolutely necessary, this should be an enum with final access modifier so there is no chance to change once assigned.

Currently isDone boolean might not make sense, given that we have split task into completable and (possibly recurring) events. This would imply that only tasks that are a Completable should be completable. Perhaps this should go in the completable class instead.

I don't believe there is a need for the toString method here since it is overridden in all it's subclasses. The body of this function will never be called. Might be better to make this an abstract method with no body.

In the original model discussed, Completable was an abstract class with subclasses todo and deadline (or possibly a deadline with optional date field). Is there a reason for the change?

Missing a period.

"Interval should be one of: ..."

dd-MM-yyyy

add requireNonNull(event);

should require non null here

Is there a need to return new EventList? Would it be better to return this instead. If immutability is desirable we can change it to only be able to access an unmodifiable event list.

Add period after each sentence, and perhaps give a short description of the param.

put the return inside the try catch block. try { return Interval....}. In general, good to avoid returning outside the try catch block unless necessary as it's easier to read (usually if you have to it might be better to abstract out the try catch block in a method).

same here, return inside try catch block.

Let's use yyyy instead of uuuu for better compatibility (with for e.g. simple date parser.)

Let's call the encode/decodeDate methods in commons.DateUtil class instead. Feel free to add the date formatter for that one so it matches dd-MM-yyyy.

This doesn't display very nicely when printed in UI, let's add some newlines and whitespace.

use requireAllNonNull method instead

great! for anyone else checking getZeroBased() method checks for negative indexes alr 😃

is creating a new project necessary?

Would it be better to move this check to TodoList for better abstraction.

I've set the observable list to track any changes to the participant list, returning a new project might mess this up. I'll check on my end and change this to a mutable version if needed.

Missing check for projectsFolder, but no worries i can correct in my next pr since it's my mistake.

blank line

might be better to name the test parseCommand_addDto_success since we are only testing the success scenario here. As per https://se-education.org/guides/conventions/java/intermediate.html, we should only leave our the expected behaviour if we are testing for all scenarios.

I think it would be better to add a check whether two objects which are equal have the same hashcode. This fulfils the second point of the hashcode contract as stated in https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/Object.html#hashCode().

Also fyi it's not really necessary to check that different objects have different hashcodes (not part of the hashcode contract) but i think it's ok to leave it in this case!

Should also check that same object -> same hashcode.

check for equals too! this is more impt than not equals as it's part of the hashcode contract.

Minor comment, maybe can use the indexes in typical indexes.

same here check for equals!

same here

check equal objects same hashcode

here too

"The update project command and the update contact command are relatively straight forward."

This line doesn't really add much value, can consider removing

typo in projecct.

Also can consider styling it. UpdateProject command is executed

typo "abd"

Not very sure what is meant by "We must ensure that the implementation of each individual command are correct.", maybe can rephrase.

Demeter's law! Probably find for now since we did similar things in other areas, but I think we should file an issue and 1 shot change all.

different deadline here

typo here

agreed. just realized the original AB3 does not use spaces consistently, might be worth addressing this in a future pr?

agree with this, will make changes and update the pr

thanks! @vevek

I couldn't get the StackPane to have 0 min height with other methods but you're right, it looks a bit odd here. Let me look into this properly.

agreed

agreed, will check the other images and relabel them accordingly too.

Using the american spelling color instead for consistency

resolved by setting minWidth/Height as necessary. This stops the panes from fully "disappearing" when resizing panes.

Current resolution updated to 1280x720 for better compatibility

Waiting for your PR @vevek, which would have implemented the required types.

Not my comments, copied from person haha

can confirm no issue here, if target and edited are not the same, and current list already contains new edited project, then the project is duplicated.

This was copied over from person class. Has same functionality as RuntimeException.

https://stackoverflow.com/questions/16241333/specify-a-value-can-be-a-string-or-null-with-json-schema

i believe null values will be converted to "null" in json, but i agree there should be a better way.

following the structure of addressbook,

addressbook contains personList contains persons.

similarly,

ProjectFolder contains projectList contains projects.

good spot should remove the space at the end

yeap surprised my checkstyle didn't catch that. thanks!

ok i might have been wrong on this, i think null should work. i'll change it on my end.

Completable utilises polymorphism. Alternatively, can have an attribute that is null if it's a todo.

This was copied over from the AddCommand. I agree with you, maybe we should open an issue and fix all of them at once?

Similar to comment above

yup good spot.

Require non null doesn't return a boolean value. Also I think it's better to leave it as this, seems much clearer to me.

This was copied over from AB3, what do you suggest instead?

I think the messages have already been abstracted out to one class for messages that are not specific to a class. We should stick to this for now, can consider other implementations in the future

Oxford comma is a stylistic choice, I think we should standardise with or without. I'll reword the to prevent ambiguity.

Using COMMAND_WORD allows the CommandWord to be changed easily.

This test checks that the createFile command does not throw an error when the file already exists

Not repeated one for persons one for projects

see above

dp refers to elevation as per comments, i think is fine for now. If not it'll be DP_COLOR_LOWEST, DP_COLOR_SLIGHTLY_HIGHER, DP_COLOR_SLIGHTLY_SLIGHTLY_HIGHER, which is not sustainable

yeap good catch, this should be changed

No need, only once we implement edit event list. (YAGNI principle)

I want to try an immutable design for events, todos, deadlines in another PR

good call

Actually, now that i think it i think it's ok to leave it like this. I feel CompletableDeadlineCard should deal with UI elements (what to display when the item is completed), and Todo class should stick to containing fields related to a todo object (boolean value of whether it's done or not).

This so in the future if we want to change how a completed todo is supposed to look we just need to change the UI

alright i'll make a quick change

Agreed, thanks for being thorough.

Changed it to contacts to be more consistent with the other level 3 heading (projects). Do you think this is ok?

ID was left out intentionally, copied this method from another handle object. Purpose of this is to compare this handler object to another completable deadline, which doesn’t have an id. I’ll look into changing the naming for these methods to clear things up.

This one a bit tricky. I decided on UiCommand since the Ui works with a generic UiCommand (it doesn't know about the subclasses).

but not sure which ones clearer

How about I edit this to mention calling the overwritten method using polymorphism? Will that be better?

Ah ur right, good spot!

yes, only can tick off using mark as done.

ID is not used in the overview section.

sure

Card to display UI elements (either tick to empty), deadline class will not do UI related stuff.

So each card has 2 constructors, one with an ID (if we need to display the ID so user can select it) and one without, for the overview section where no id is required.

Why not have ID's for both?

I'm afraid users will get confused. I also don't want the users to be able to run commands on tasks in the overview section, as this might lead to confusion.

seems like the ci is failing due to checkstyle issues


ERROR:../docs/UserGuide.md:213: no newline at EOF.

WARN:../README.md:15: trailing whitespace.

Just realised you accidentally changed the user guide instead of the readme haha! Might want to edit the title 😃

Updated user guide with view and undo/redo command and changed some references of AB3 to CoLAB. See #9.

@samuelfangjw Looks great! Please add the changes you have added to features to the table of contents.

Done! Forgot to update the table of contents when resolving the merge conflict. How does the latest commit look?

Tutorial PR irrelevant to project.

Which images are you referring to? @vevek

@vevek XR claims credit for finding 😂

LGTM.

haha thanks but it's still a draft pr! i'll make some edits tmr and change the pr status when I have something decent 😃

Are you able bring back the v1.1 milestone?

can add to v1.1 (there is a closed tab when you click on milestones). if someone wants to do it now i think is ok to change the milestone to v1.1, else we can just throw it to next iteration.

let's add a confirmation message in case users accidentally clear all

Will figure this out one of these weekends, need a pc to replicate bug.

Need to merge #51 before merging this PR, also need to request to use TestFX library on the forum.

latest commit fixes #58

Team decided not necessary.

Looks good to merge. Minor errors. I suggest adding comments to all the methods now so that it's clearer and won't be a bigger hassle in the future.

Which methods are you referring to? There is no need for javadoc comments on getters and setters. Method with @Override tag inherit javadoc comments from superclass, and there is generally no need to add a javadoc comment unless the behaviour has been modified or there is something of note. e.g. equals, toString, hashcode.

I will work on tests and other features over the next few days. If no major issues suggest we merge this soon so other team members can start working on their features.

Lowered priority to low as this does not affect correctness of current code and is not time sensitive.

Great! Please try to finish at least some of the tests, our codecov score a bit low at the moment. Yes, the UI is not ready yet, I'll do a simple one up later.

I'll take a look at this in a couple of hours, think I saw a few oddities but have to take a closer look.

In addition, as pointed out by @Eriksen2411 there should be a blank line between the first line of the javadoc and the params.

Ready for review, currently UI a bit lacking but I think it would be better to improve on the UI in another PR. As for tests, a large section cannot be covered now because they involve the UI. Will be able to do it once we merge #61.

Tests were failing because tests were adding and modifying the same project list. (our list is not immutable, maybe issue for another time). I've solved this temporarily by creating new project for each test.

Ready for review

General hashcode contract states that

When hashcode is called repeatedly on an object, should return same result unless object has been modified.

Two objects that are equal (according to equals method) should return the same hashcode

Different objects do not have to return the same hashcode, although it's good if it does so.

Wah I never knew there is such a contract. Will do it tomorrow!

For the third point I think it should be the other way round. Objects that are not equal should return different hashcodes as far as possible.

I think it should still be pretty safe to check for different hashcodes for different objects since it should be quite unlikely to collide. Do you think we should remove this check?

Yup, no harm checking since very low chance they collide (int can only represent a finite number of things!)

Some minor style issues.

Thanks for pointing this out, wasn't aware there was a style guide for md files. Will look through the style guide again and update.

Let's standardise contacts for contacts in the contacts list and group mates for contacts in the project.

Updated, please take a look thanks!

Currently, I've changed the UI elements to reflect the new groupmates name. However, most of the methods, commands and classes still reflect the participants name. I think better to change in another PR, I've created an issue for this #154.

Done for now, will continue looking another time.

Perhaps standadise it to ITEM_NAME.

Same for here 😃

Our list takes in an optional parameter, so may consider removing it from this line.

our list should be able to in an optional field, location.

Should consider changing edits to updates 😃

Should be deleting an item instead 😃

Check google docs for delete method for StoreMando 😃

One suggestion I have for these JSON files is that maybe we should rearrange the sequence to match our display sequence to make things more neat and tidy 😃 so instead of the current name, quantity, exdate, location, we put name followed by location and so on based on our current display.

Some variable changes are still seen here and there

For this part just let Fazil know to change this to standardised locations, because this one he may overlook thinking that it's correct cause Apple Banana etc are what we using for item names.

For this when it is showing all items will it also work as normal?

It May be too technical for a user guide

Perhaps its better to say 'Expiry date of an item is optional'

Why is there an additional catch NullPointerException?

understand that for this file the sequence in which what is placed first does not matter, but for readability and consistency of our code base we should perhaps standardise it, following the google docs sequence

I think "Quantity should only contain numbers..." will suffice, can remove the "numbers" after Quantity

This may be a violation of Law of Demeter, cause browserutil calls a method that calls helpwindow's string. Maybe can consider shifting the USERGUIDE_URL to BrowserUtil instead 😃

Instead of calling 2 booleans, maybe inside BrowserUtil class create a boolean to check for these 2 booleans, since at this class we don't have to know the details. Maybe can create a boolean called canOpenBrowser 😃

Agree with this, but if its functional we can leave this to v1.3 as well. 😃

Personally I will prefer trimmedArgs = trim()

Then in the condition, we use equals ignore case instead.

What's the set of names we following from now on? For the original ab3 it was ALICE, BENSON, CARL, DANIEL, ELLE, FIONA, GEORGE. What's the pattern for ours?

Agree with kums!

Agree with Fazil!

I feel that this shouldn't be an exception condition because we should instead just ignore whatever that's not the first arg. So reminder 6 haha should just take it as reminder 6. on the other hand reminder haha 6 shouldn't work but it has been considered in ur try catch method below.

Whats the reason for parsing it as long instead of int?

With my suggestion below, this will become unnecessary

for this whole parsing right, perhaps you may wanna think about changing it to a prefix kinda thing. Say reminder d/3 reminder w/2, with that you will be able to use preambles and also prefix isPresent to do most of the checks here and I can foresee the code to be neater 😃

how will the >a name = "delete">>/a> look like in the user guide page?

perhaps change to 'NUMBER' instead, because in our user guide only command words are small letters in format I think 😃

will 1 day and 1 week work? or even 1 must be days and weeks?

if 1day and 1week work then that's the best. Else here's my suggestion:

change the implementation such that 1 day and 1 week will work.
make it extra clear in the user guide (1 more sentence behind it) to emphasise that day and week will not work. 😃

There will be a need to update this part of the user guide because now there is quantity asc and quantity desc 😃

also this, it should be expirydate, emphasise to the users too that it is case insensitive. 😃

There will be a need to look into this section to double-check, examples will be reminder and sort. 😃

Can we add one example for weeks as well? 😃

I think u just need the first line to be in if else statement, the rest should be outside as one right? 😃

Maybe instead change it to 3 positive path conditions in if elseif, throw in else. 😃

maybe change from 'remember items' to 'keep track of items' 😃

Should it be 'make the most of StoreMando' or 'make the most out of StoreMando'

What will >a name="start">>/a> show in User Guide page?

Will this hyperlink still work after u added '3.' for 'features' below?

Can we make this example more legit, perhaps change to l/kitchen cupboard 1(somewhere more normal) ... e/2021-04-01(a date that is more updated as of our user guide edited date)

Is there any part mentioning that '[]' equals to optional? because even though tag has '...', in edit for example most of our parameters only have '[]' 😃

I think this info about the index being positive is not required because it has been stated very clearly in the sentence before this that 'The index refers to the index number shown in the displayed item list.' 😃

same issue with the previous examples, which is likely to be the same for the rest of the examples. Please look into this! Thanks! 😃

Same reasoning on this just like the previous comment. 😃

Why is this not bold anymore? How will it look like in the user guide page?

u did the assertion then here no need the if alr right 😃

Fixed 😃

Resolved. 😃

Nope it was not fine, thanks for spotting!

Yup I will remove this file now. 😃

Better Model class puml is not in use as of now. 😃

It looks like this now, the hidden arrows wont be displayed.

Thanks!

This will need to look into it further in v1.3b, we will take note of this! thanks!

Yup thanks Fazil!

Will add more test 😃

Alright I will try 😃

Do remove the trailing whitespaces in order to pass the build checks

Is this needed?

I think would be nice to remove the comma to standardize

Additionally, can you help to remove Line 8 as well (I think not relevant)

Sozz, i realized the bolding looks a bit weird, would be good to keep the bolding within the same sentence.

For this portion, I think would be better to use the one in @weixue123 's branch, so might need to merge it in first.

refer to this portion

Would be nice to have spacing between CHIM and the full stop

Are we keeping this portion? Can refer below for the modified command in @weixue123 's repo

code portion

Use the static method getCheeseType instead of the constructor (wanted to make the CheeseType a bit Enum like so we can also use == for comparison)

Add whitespace

Would be good to leave it as private and use the static method

Do specify the type of each date, currently it doesn't show what each date represents

Same for order

Any reason to prefer this over null?

Is it possible to increase the padding? The cards seems a bit too packed

Would it be possible to use enums instead?

add assertion error here

Remove debug statements

Possible to extend DeleteCommand to do more. for e.g. help to do the basic checkings such as index out of bounds and so on. If not enough time, its fine

(must be a valid phone number)?

use .equals instead

would be good to do new Phone(customerToDelete.getPhone().value) to ensure that it is comparing by value and not just by reference

Would you be double calling the method (because you call it in Delete___Command.java as well). Would suggest that you call it from outside instead of here.

Possible use case is, in the future if we allow deletion of customer with all of the customer's order, the panel should only change to the customer panel and not the order panel

Just a suggestion (but up to you), you can consider using "Assigned" and "Not Assigned" instead

use .map(x -> x.toString()).orElse("-") so that it does not throw null pointer exception

sorry forgot to update this in properly in previous commit

Additionally, do add isAssigned to .equals as well.

Do the cheeses need to be updated to have the isAssigned attribute?

yeap something like that

seems like a static method because it doesnt seem directly related to the object

Would be good to set the panels inside the commands instead.

Was thinking in the future, if we for some reason modify a cheese's ID, and have to update the order too, then the view will be changed to the order panel (instead of the cheese panel)

The logic is good 😃 possibly can do 2 things

Sort by maturity date too (to ensure that the assigned cheeses are matured)
Can just use a counter to count the cheeses instead of using decreaseQuantity(), seems a bit sketchy haha
I think the setCheese should be done else where maybe in the command (using model.setCheese()). The getUnassignedCheeses should only get the cheeses without updating anything yet

Additional whitespace here

I'm not sure if it would be benefitial for us to set quantity's value to private (because it is already final so we can't rly change it anyways). But if you do have a purpose for it, then I'm alright with it

Hmm, curious, does equals on the hashset not work?

I think I found out why (my part also affected by this), have to make sure that all the ids have the same hash. Will be updating in my branch

Was thinking that CheeseId should not be doing things for the sake of the stub (i.e. not know of the existence of the stub). Can just specify that it returns the next Id?

Add newline before Example

Same here

Do update the toString method for cheese (now showing optional.empty)

Currently, there is one issue with this. Steps to replicate:

Use find command to Find Customer B
Delete Customer A
The customer phone number provided is invalid.

I would suggest to use the getCustomerWithPhone to find the customer instead, filteredCustomerList is a bit unreliable

Instead of doing this, would it be possible to have a method, e.g. getOrdersForCustomer so that you don't have to change the filter all the time? I'm afraid similar bugs as the issue mentioned before may appear

Same as the comment in #37, don't think that we should change the UI portion just to delete the cheeses (although functionaility-wise it is exactly the same, just the organization is a bit different)

Would suggest to use the getCustomerWithPhone method in AddressBook instead of filtering the whole list

Do resolve conflicts

Same here

this not needed here? seems like a repeat in OrderPhonePredicate

Will bring up a proposal for this in the meeting

My bad, the class should be used in Order, will add in in next commit

3 Things

This portion of code is to test the EditPersonDescriptor (which is not a part of the model, but only used inside Commands, hence this should be in the right place
Regarding invalid dates, will add a few in the Dates class
Regarding invalid cheese types, currently there is no way for a cheese to be of an invalid type (because it just needs a name), unless we check for empty String

Would be nice to standardize but might not be necessary (will have a lot of existing codes to update)

Will bring it up in meeting tmr

I think would be good if it is the 1st case (less bugs when they smoke test)

Which would you suggest to drop? If we want, we can just keep expiry and manufacture date (then I can add one more case in the AddressBook to ensure that the Cheese's manufacture date must be after the order's order date)

I think this might cause some trouble on our side cause imagine if Order shows all the cheeses that are assigned to it (and also cause order needs to check that the cheese is of the same type).

I think, we can possibly disallow cheese deletion if the cheese has been assigned? (cause I think delete cascading will be bad for this)

Will discuss on Saturday

The AboutUs image is not working... I think you would need to rename it to laurenlhy.png (currently its named laurenlhy.png.JPG)

Hi, you can check out the branch telegram-github-notifier on the master repository. It seems that the build isn't working there as well.


Forbidden: bot can't send messages to bots

Actually do you want to squash some of your commits, before merging. But other than that, everything seems fine.

Yea, I think would be good to squash to about 3 commits

I rebased the branch, gonna merge it in soon

Closed in favour of #20

Do remember to modify your code to support the toggling of views once #20 is merged in

There is one class of ModelStub with no useful methods, and the rest of variations are extending from it and overiding methods important for testing. We could make it abstract though. What else do you think we can do. The idea is we have two kind of test suites: 1 with using the actual ModelManager and 1 using just the stubs.

I probably can't think of a better way to do it, but I'm afraid in the future it might get a bit messy because of the so many variations

Remove empty space on new line >_>

Formatting!

Swap the parameters to be consistent with the rest of the assertEquals

nitpick: that should be named other iirc

No need else clause here. Can reduce the indentation level.

Its not exactly a nameString. Rename to searchString

Got better name? I think calling it anniversary would feel better other than date.

return time == null, no need to store another instance variable.

Can just check using time == null

No space between Event and (

nitpick: no need else clause, just return

Style violation, supposed to be 8 spaces from the preceding line.

I think better to store as LocalDate. Can use #41 dateUtil to make it easy.

Use DateUtil.fromDateInput(...) to get back a LocalDate. Use that to create Birthday. This will not change the tests that were already written.

Wrong comment. Should be {@code Birthday}

Probably can do the dateUtil parsing here too

Yes, we should still do it. I added some comments to the test files. Essentially, when we create the test objects, we parse the dateStr to a LocalDate using the util methods. i.e. withBirthday(...) takes in a dateStr, parses that string and returns a new Birthday with a LocalDate in it.

I think this should be in the present tense. if they are not in it

Can be shortened to


return Arrays.stream(arr)

  .map(ParserUtil::parseIndex)

  .collect(Collectors.toList());

I think there should be a space between the arrow heads. updateFilteredPersonList(x -> group.getPersons().contains(x))

Wrong indentation, change intellij settings

Having a parameter here is out of the norm. Can add a comment here saying that it requires the actual personList serialised from the persons json file?

Multi-line for readability. I think the lambda has a spaces enclosing the arrowhead.

Not optional parameter. This line should be + PREFIX_INDEX + "INDEX"\n

Should we have the command name be del-date? Easier to type a shorter command

I think this could be better.


LocalDate xRelative = x.compareTo(now) < 0 ? x.plusYears(1) : x;

LocalDate yRelative = y.compareTo(now) < 0 ? y.plusYears(1) : y;

return xRelative.compareTo(yRelative);

Can we have date.setText(personEvent.toUi())? Makes it easy to test the formatting later on

We should store the localDate here instead of taking only the day and month. Easy to change formatting later on.

Should have a toUi method here to do the formatting using the LocalDate formatter methods.

PersonEvent should have no knowledge of Birthday and Event classes, put these two helper methods in their respective methods. They could be named toUi() too.

So for here, it will just be person.getDates().forEach(event -> personEvents.add(new PersonEvent(event.getDate(), person, event.toUi())

person.getBirthday().getBirthday() is very strange >_>. Can rename to the last getBirthday() method to getDate()?

Rename to getDate()

Ok, got it. Lets leave it here first

👍

Is this still temporary?

Should this be date.toUi()?

Possible to create another component to store the profile picture? The same code is used in PersonDetailsCard and PersonCard.

I think you have to write the b/ prefix stands for. I think we can leave birthday there. Users should know to supply a date from the examples below.

Would this be better? Can optionally provide a group name to list all friends in that group

I think we should still put Birthday here

Would be good to write docs on the specifics for Event and Picture as it is not clearly shown in the class diagram. Like the multiplicity of Person to Event (1 to *) and multiplicity of Person to Picture (1 to 0..1)

I think would be good to document something the rationale for this

Ok, btw I was talking about the usage of a single Person in the detailedPerson observable list. That one probably need to document the rationale for it.

Person is still immutable, the setters are not changing the Person itself. Will change it to withDates instead of setDates because the set prefix is commonly associated with mutability.

This should work for asdf.tar.gz. .gz will be extracted from the function and validated.

I think its ok here because we are deleting the filePath of a picture.

Nope, this is still used when the Person being initialised has no meetings and no dates.

yeap, my bad

👍 too late alr

Lgtm!

Would be good for Event to use a LocalDate instead of String to represent date.

#41 fixes this

Please add "by their name". Your sentence is hanging right now

literal "abc" should be the constant NAME_FIRST_PERSON

please put this in a final variable like final String nonExistantName = "WHATever it is";

I think? this is fine, but I'm not certain. Double check best practice maybe?

Good good, Nice to see you not having magic literals.

Maybe put in a variable with a descriptive name first like String nameOfPersonToDelete

Should this be an invalid name, or would rejecting a number suffice for this test?

LGTM, although I personally would prefer 'their' instead of 'his/her'

it should be delete NAME, not delete name. All Caps for variables I think

Class is wrong and variable name is wrong

AddressBook ab should be ContactList cl

getTypicalAddressBook should be getTypicalContactList

addressBookStorage should be contactListStorage

Method name again

readAddressBook method is wrong

method name is wrong and probably wrong for many test case code

variable name differentAddressBook

comment has reference to ReadOnlyAddressBook

value = "addressbook" is probably wrong 😦

addressBook variable name

Class name AddressBookStorage

Comment is unchanged

method name

variable name

bad comment

variable name addressBookOptional and method name readAddressBook

method getSampleAddressBook

method name

comment is bad

Also method name

variable name addressBookStorage

getTypicalAddressBook

method name

saveAddressBook is bad

setAddressBookFilePath 👎

method name

getTypicalAddressBook is a thorn

method name 👎

method name and variable name 👎

I think this one and this one alone should be unchanged given that it references an external project

More bad method names and variable names

comment "Converts this address book into the model"

Attributes are "Car" and "CoeExpiry"

shouldn't this be resumes from step 2, like in UC1b below?

what defines a conflict? overlapping datetime + same name? or overlapping datetime + same doctor? or both? or are there other cases where a conflict will arise?

In the same vein, how specific should we be for Use Cases?

Think all the 1a's and 1b's can be summarised somehow since its being repeated in all use cases, but I'm not sure how to handle it. does *a work? or use inclusion with a bigger use case? -


1. user enters a command

2. system reads the command and executes the command 

1a. invalid command

1b. invalid subcommand

might want to consider changing to something like this?


steps 1a1 to 1a2 are repeated until command entered is correct/free from errors.

same thing for the other loop-style/invalid input by user use cases

1c, 2a looks to be repeated as well in edit and find (1c), and edit and delete (2a)

@AY2021S2-CS2103-W17-2/developers

should it extend Exception or RuntimeException? any particular reason for making this an unchecked exception?

same for NegativeOrZeroDurationException, AppointmentConflictException

is this an intended mention of addressbook.json here?

maybe consider a naming more similar to UniquePersonList.java, and same ordering of methods as UniquePersonList.java

not sure whether this is allowed under coding convention for spacing

this doesn't look like it fits the regular format of implementing Parser>DeleteAppointmentCommand>

think u may have missed a param INDEX?

command should be something like edit-appt 1 pt/2, which means:

edit first appointment (right side of GUI),

change the patient in the appointment to the second patient in the patient list (bottom left side of GUI)

check out editPatientCommand regarding this.

i don't think this is true, if this were true then each patient would only be able to have 1 appointment at any point in time.

why allMatch? this doesn't sound like it would work.

with allMatch(), wouldnt it mean all appointments in the appointment schedule have to be the same appointment for this to return true?

this sounds like it should be anyMatch instead, in which case the original contains() should be used

i think leaving as List>Appointment> may fit the current coding convention better, All previous commands like DeletePatientCommand, EditPatientCommand uses List>Person> as well

agree,

rationale is that for a scheduling conflict to happen, either patient has overlapping timeslot, or doctor has overlapping timeslot,

think this method wouldn't work, and hasConflict should be used instead

same issue here with allMatch(), should use hasConflict instead

i think the issue is that the current editContains is using allMatch(), which i don't think is correct?

from my other comment on this, all appointments in the schedule have to return true for equals() for allMatch to return true, which doesn't seem to be the right idea

this code would not work if patientIndex didn't exist. Should handle it like lines 104-106: ie. something like


Person patient = editAppointmentDescriptor.getPatientIndex().isPresent()

        ? displayedPatientRecords.get(editAppointmentDescriptor.patientIndex.getZeroBased())

        : appointmentToEdit.getPatient();

this method name sounds like you're converting a Set>String>, instead of converting to a Set>String>. Would recommend to change to convertToStringSet or something along those lines

is there a need to 8 spaces then 8 spaces again? im not too sure about the coding convention

what's the rationale behind trimming? code looks like it should work without trimming right?

programming to an interface is better coding practice if you don't need to use any ArrayList specific methods: ie.

List>String> xxx = new ArrayList>>(); instead of ArrayList>String> on the LHS

combine?

String keywords = argMultimap.getValue(prefix).get();

this may be just me but i feel like doing the object creation on another line feels v messy...

i'd rather do Collections.addAll(tagKeywords, listKeywords(argMultimap, PREFIX_XX));, but its up to u!!

think this portion should refer to both appointment schedule and patient records, not just patient records

is there a need to separate the predicates PREDICATE_SHOW_ALL_PERSONS into Patient and Doctor for better clarity? since in this command we are supposedly only showing all patients, not all persons

same question with the separation into doctor/patient for this

believe this may change to just Predicate>Patient> if we don't use the Predicate>Person> in Model.java (change will populate downwards)

abstract class?

propose changing the name of this class as well since it only affects data for patients.

on a side note, i think we will need to create a sample appointment data util to populate sample data for appointments as well. will add this in the issue tracker.

may be able to make this abstract as well

Accidental use of Patient here.

can probably remove the initialisation and stick with only the declaration if abstract class is used

should change AddressBookStorage here to AddressBookStorage>Patient> to avoid future confusion with AddressBookStorage>Doctor> when it is added into this method

to change naming of the json file itself, same for the other test jsons,

unless intention was to leave it as PersonAddressBook.json because should be clear enough that it is meant for patients because it is in a JsonPatientRecordsStorageTest folder. (but i still think renaming would be clearer)

to update comment

small issue that doesn't affect anything but the param name is still addressBookFilePath

consider refactoring method names?

I don't think so? I think this should be a class that tests for all versions of addressbook? because this would correspond to the main class AddressBook.java, which is for both doctors and patients right

yup i think so

don't think so, logic is we don't want any patient commands to affect appointment schedule, so we make a new typicalAppointmentSchedule here

still missing some sections to UG, like the summary table, so will not close issue yet.

fixes #6

duplicate PR with #26

you can perform the CI checks locally by using .\gradlew build

is this PR still relevant? @icytornado

to solve:

recommend 2 prs:

raise exception if appt list has the patient being deleted,

- recommend to add `--force` (or whatever) parameter, force delete feature: 

- if `--force` is specified then cascade the deletion

3 ways:

>s>1. change patient class to have mutable fields (and potentially appointment class)>/s>

>s>2. couple the patient class to appointment class such that edit-patient will also perform edit-appt>/s>

ID (or UUID)

create doctor that extends from person and relevant methods

Actually just thinking what would happen if we put the time as 23:592. This is because when trying out the iP, I noticed that one of those that I am reviewing failed this test, so you could include 1 more test with this value to ensure that adding extra values to the timing will not cause any unexpected behavior ah.

Yeap! I think that the test case should fail with these values.

Hmmmm honestly I tot that changing the values for amy bee here will will affect places like the CommandTestUtil.java as well and fails the test case. But seems like it passes the test case. Seems like they are not linked then. But just in case next time there is some error for this, we might need to check back ah.

Should we comment out find_session here instead of deleting them?

Ohya actually is find_session part of v1.2? Cos now that I think about it, its quite weird for find_session to include the Index. Like it wouldn't make sense for me to look through the whole list of session and try to get the individual index and then run find_session again, cos the information we give from the session list and find_session would be exactly the same. It would make more sense for it to be just find_session s/STUDENT_NAME. But if this is not under v1.2, I think we should comment this out first before we discuss again?

As per the above, this seems to follow the find_session s/STUDENT_NAME instead ah, should we comment this out first?

Ah ok! Didn't see that the comment header start at line 114.

Great!

Hi, just want to check as the previous line ": Deletes the student identified by the index number used in the displayed student list.\n" already has an \n, does having an additional \nParameters here cause anything weird on the UI? Might be better to just keep one.

Hi, just checking if this is something extra that you did that isn't really related to the UI. Can I check if the purpose of this code is to check if the equals method works as expected?

Hmmm using student instead of s, might be better in this case

Hmmm is these commented out withsession intended?

Looks good! Just wondering if there could be comments here similar to ModelManagerTest.java equals method, to better allow the others reading this code to understand the test cases here better.

Hi, just wondering, since the AddressBookParser is in `` in the next line, we could possibility AddressBookParser here for consistency as well.

Haha based on what jonah said, actually why you changed also ah. I tot its part of Logic, isit because you did the delete_student command previously?

Hi, perhaps you could copy the diagram over to a folder with your name instead and restore the original file ah.

Hmmm I think line 154 to 155 and line 157 to 158 are duplicates ah.

O.o damnnn u did both. Haha ok sorry. Let me go approve ah

Actually I think this part is abit confusing for me, aliceInTuition when calling new Tuition() has a sessionIndex of 2, however, we assertNotEquals that getSessionIndex() is 2, isit because of the difference between the zero index and one index?

Same comment as the getSessionIndex() above.

I think there is additional space on to LogicManager, but small problem ah.

Actually you could include ../style.puml instead, so you don't have to make a new copy of the style.puml in your folder ah.

Only one comment, I was thinking the indentation for the addSession(withSession previously) was abit weird and should have the same lvl of indentation with withStudyLevel instead. Do you think it would be better if we did that?

Actually outside of scope, but this comment here seems abit unnecessary with ////, I think we could just use // instead, so that it won't be flagged as a "bug" during PE dry run.

Yo actually what is this for ah?

Hi, understand the the testing is still a work in progress, but I think it could be better if we didn't use System.out.println() when we are merging to master. Using assertions might be better.

LGTM. But was thinking if we can be more explicit that the interval is in terms of the number of days.

Hi was just wondering what could does >p> mean. Isit a typo?

Hi, just checking, for this implementation, we run super(session) to create a standalone session out, but then it doesn't seem to keep a reference to the created session anymore. Can I check if that is the intended effect?

Hmmmm already, actually didn't use super often so not sure how it would work out. We could keep this in mind and when the rest of us work on features that uses recurring session, see if we are able to perform all the actions we want, even w/o keeping a reference to it ba.

Actually not sure if other groups will nitpick this, but we could give s1 and s2 a more meaningful name such as sessionDate1 and sessionDate2. Will be longer, but at least we prevent other group from nitpicking these small bugs ah.

Hi, maybe we could add a //TODO: tag here, so that we would remember to come back here in the near future and work on this.

Also I am just wondering if there could be a way (Probably through a command) to add a recurring session into to application, so that the reviewers can checkout and run the PR to perform manual testing as well, to better understand your implementation. Not sure if you want to work on that first before we reveal and approve this to ensure that the implementation passes some light testing. If not it might be hard for me and jonah to test if our feature works if we couldn't add recurring session inside to the addressbook to test.

LGTM, I think "40" is missing a space, to be "40 "

Maybe you would want to pull upstream first and merge inside, I believe that choon wei added a functionally that previously adding session that clashes. (E.g. If a session that starts on 2020-01-01 12.00 that last for 120mins, we can't create another session that is between 2020-01-01 12.00 to 2020-01-01 13.59) Although, with recurring session, this is much harder to achieve, because we need to take into consideration from the start to end date, if adding this rec_session will clash with future individual sessions as well. Not sure if you want to push the 2nd part into a separate issue as it seems like quite a big implementation.

Ohya one of the possible issue is that I think the storage has no way of identifying now whether a session is a recurring session or a standalone session. I tried creating the recurring session, close the app and open it again, and the recurring session becomes a standalone session now. Might need to take the storage into consideration, either in this iteration, or as a separate issue.

Hi, was just wondering if the ArchitectureSequenceDiagram has been changed to match any command, if yes, do put it into a folder with ur name and rename the image src accordingly! Thanks!

Actually I think we do not have to edit the tutorial codes ah. It probably shows up here cos of refactoring.

Yeah, I think as per the tutorial yst, having a class diagram of a single class might not add value in this case.

Ok ah I will change the values in the file back to the original code, I have no idea why this file is in our directory also.

Updated.

Waaaa nice catch man! Updated it accordingly already!

Ok good catch! Have updated the test to include these 3 cases.

Haha alright! Updated

Haha LMAO, but I think its fine tou, it allows the reader to understand that its filtered, and can be the full list also.

Alright, updated to Listing students' emails based on current list instead and also updated the TOC

Haha yeah I tot of before, but then I think at most there will only be 2 commands, one to get monthly fee for a particular month, one to get past 3 months fee. So not sure if it should be a section by itself. Although I think doesn't hurt to change ba. I will make the changes first then.

Actually not sure if its ambiguous, I think if we really want to be specific "Student fee for a particular month and year" would be more accurate. Will update to this version first.

Nice catch!

Took the comment and added the information

Nice catch! Updated as required

Hmmm ok, seems like ur use the filteredStudentList in your explanation also, so I included them as well ah.

Ah ok! Seems logically, updated it already.

LGTM.

The Student Commands need to be updated after the addition of the Tuition class #40

Ok great! Should we merge this first? Since we are doing this in phrases, I think it makes sense that we merged inside after a functionality is done, compared to merging it when tuition + session + student is done together. I believe choon wei would have to reference this changes to work on the tuition class as well. Delaying the merging could mean that there will be alot of conflicts when we ultimately try to combine everything together.

LGTM!

So, this will only be an issue if the user is using a 32-bit system?

Yes I did a rough search and it seems that it is possible that 32-bit system running java might have the 2038 error. Hence I think setting an upper limit to >=2037 would be a good compromise.

If I remember correctly, the "As a" part don't have to be a user. So in this case maybe we can do: "Cake Collate/Program". But, I tried to find this in the lecture notes/textbook and couldn't find it.

So, maybe we can omit the "So that"/benefit part instead of "(obvious)", since it was mentioned in the notes benefits can be omitted if they are obvious.

The full-stop in the benefit part caught my eye haha. Maybe standardize across the DG~

Maybe instead of "track", "execute"? Or something along that line.. Because we aren't really tracking the deletion, just actually removing it from the database.

Noticed a "/" at the end of the benefit part.

Maybe "re-adding" or "adding"? haha

Noticed full-stop here as well. Same for a few below this~

Noticed you used capital "V" for "View". Maybe standardize and use small caps. Same for the benefits part in line 264 and 267 "My" and "Details".

Maybe can leave out the benefits part too.

why got another add nub

Not sure if the AB3 allows for 2 p/

I think it does but maybe to make it clearer in the UserGuide, just put one phone number

Can you add a full stop haha

Need to change "person", same for line 29

Maybe needs changing/removed

Assuming you're asking a question?

I think it would be good to have a way to differentiate or find out if two HelpCards are the same, regardless if we ever use this equals method.

Same thing, need to change "persons", same for line 29, 48

Maybe add a brief javadoc?

Hmm, just from looking at the files changed, I can't figure out why you abstracted out fillPersonListPanel()...

Same thing here, but I am guessing because of some dependencies you needed to setRoot(root) before you setController(this)

May I know why you are deleting this? Is it because this method is not used?

Hmm.. By right the data shouldn't change while the user is in the help panel. But it wouldn't harm to insert redundancies so that the program won't break if for some reason data was changed during the transition. So, I would recommend using this fillPersonListPanel() to populate the personlist panel.

Ahhh, I see. We were told cyclic dependencies are bad practices in 1101 right? While its a bad practice I think it is still used haha if you take a look at w8 topics the first video for drawing class/object diagrams, they have cyclic dependency also: box and lid.

Maybe if you want to, you can change the resetMainWindow method in MainWindow to a static method then use a static call in HelpCommandPanel. Not sure if that is still cyclic dependency lol. Else if you really want to resolve it I guess you need an association class.. So, goodluck!

"orders" ? instead of "persons"

Same thing here, "orders"

😮 why is this here? Thought it is supposed to be changed by Pavitra's PR

If you're changing such that user can choose the range of dates then you should remove "is within 3 days of the current date" right.

You can maybe merge this two if you are not trying to differentiate the reason why there is a parse failure (since you are throwing the same exception). In a way the error message will be less meaningful but it is what the original coder(s) did too for AddCommandParser etc. We discussed this briefly during our last meeting when @pPris brought it up. I think for now we are KIV-ing and maybe changing it if we have time.

Should be "order"

Same for other usages of "people" and/or "person" for this file

Remove this if its not used? 😃

He probably needed the request here to create an order item lol. Although it is possible to overload the Order constructor.

Maybe for extensibility. Having a prefix in place now means in the future if we want to add another prefix it can be easily done.
Maybe so that user can input more than 1 request per command. Without the prefix, the user may have to use the command twice to input 2 different requests.

I think it's fine leaving it as it is now 😃

"diabetus" is on purpose or typo? lol

Remove this boi

CommandException should be removed

Just wondering why you use .value here while you used .toString() in L41?

To test "no parameters" you should include all other required fields and only leave out the parameters. I'm guessing you mean to test for the prefix here? Since you are testing index below

Same for this, you should only leave out index and include prefix.

Yup definitely! If I recall correctly Benson's details will be used specifically in some tests. So since you are adding a field as well, take note of those specific test cases that tries to extract Benson's details ya. Probably don't have to do anything about it if you didn't miss anything ~

Good catch! Will do.

Yup, there are. Nice catch again:)

Add a OrderDescription class to seedu.address.model.person and edit the Person class to reflect added field.

LGTM~

LGTM 👍

LGTM 💯

LTGM

LGTM

LGTM, can always add more next time.

LGTM 👍

LGTM ~

Code Quality sounds more like a responsibility. You could put "Developer, VS Code Expert" for your role.

Should these @return and @param aliasName have descriptions?

There are similar cases in this and other files.

Missing method comment?

These look like they shouldn't be here.

Are these missing JavaDocs?

Is this missing JavaDocs?

Is there a link to this source? Also, if you understood and adapted it, consider using

//Solution below adapted from ...

Reference: https://nus-cs2103-ay2021s2.github.io/website/admin/appendixB-policies.html#giving-credit-for-reused-work

Saw this in a few other files.

Should there be a newline between these two methods?

Could you add descriptions to these tags or remove them?

What is object?

Please standardize the javadocs format in future to follow the style guide. Will approve first because it's non-breaking and we should merge soon.

Perhaps this could be instead shown as an arrow returning to the user

Consider using MODEL_COLOR instead of LOGIC_COLOR since this is part of the model component.

Should the return arrow be dashed and the activation bar disabled when returning?

Perhaps

can then be executed by entering the Alias instead of the full or partial command.

should instead be

can then be executed by entering the alias instead of the full or partial command.

because the user is not typing in Alias but rather the alias that they specified. It's a small semantic point.

Are these comments supposed to be here?

There may be a missing closing bracket here:

if () then ([all parameters are valid)

The tense seems a bit off here.

Maybe this

If the input begins with an existing command word, parses it as one of those pre-defined command.

should be

If the input begins with an existing command word, parse it as one of those pre-defined command.

(italics added). Similarly for the rest of the points.

Also consider placing this description above the activity diagram, with a transition to the diagram. E.g.

The following diagram illustrates this flow.

DIAGRAM GOES HERE

Consider labeling the arrow returning from UI to user with display commandResult instead of this:

I'm not sure but the new keyword may not be appropriate in a sequence diagram. Consider this example from the textbook:

Maybe change return result to return commandResult for consistency with the related return statements below.

Should this be an IssueList?

This activity diagram appears to have the condition within the branching diamond. However, this may not meet requirements for this module. Consider moving the branching conditions out of the diamond as guard conditions in square brackets.

Agreed. It may also be good to list out pros and cons for each possible implementation.

This looks like commented-out code. Was this intended to be here?

There may be a typo here. Did you mean something like this?

Checks that room 03-100 is not occupied by anyone.

There may be inconsistent tense here.

The problem is reversed if the parent-child roles were swapped.

Maybe this could be changed to either

The problem would be reversed if the parent-child roles were swapped.

The problem is reversed if the parent-child roles are swapped.

Consider adding a fullstop at the end.

It may be good to give a short description on what this example does.

Good catch, thanks.

Hmm good point. An optional frame might suggest that a command being unsuccessful is the "happy path". My original intention was to convey the fact that unsuccessful commands will not be recorded in the command history, but I guess an activity diagram or a note in the sequence diagram might be more appropriate.

Thank you, I will consider this suggestion seriously for a future update.

Hi Colin, could you re-open this PR to the upstream master branch to follow the forking workflow?

As agreed in our team meeting, we will be splitting work by feature, not component.

Closed in #63

Closed because no longer relevant

We may want to break this up into at least 5 separate issues since each member is expected to update the DG. That way, each member can close the relevant issue when they are done with their part.

Further, I suggest we break up DG update issues by feature, rather than by team member because a DG update per feature seems like a more correctly-sized unit of work than a DG update per team member or one monolithic update consisting of everyone's updates.

I would like to discuss if the command history should also include failed commands. I think there is merit to mimic the same behaviour where the invalid command can be access via up and down arrow keys so that a Power User can edit the invalid command rather than retype it all.

If we do go down this path, I think a possible implementation is that each history entry will keep a field to keep the corresponding command used. Thus, if the field is null, it indicates an invalid command, otherwise it indicates a valid command.

Hmm, thanks for the suggestion. SunRez currently does not consume the user's input when a command is invalid, so a power user can already edit an invalid command easily.

Split into 5 separate issues that can be closed individually by each member.

i agree with yi hsuen. But for final consensus, lets discuss in the meeting as there is no rush to do so

Yes I believe so

invalid? Since these are AddressBook tests and everything should be related to ModuleBook3.5 now

same issue? Maybe the pull from upstream wasnt done properly as the current master has no invalidPersonAddressBook and is invalidTaskModuleBook

testing for AB3, not ModuleBook3.5 😕

LGTM! but i agree with yi hsuen. should just be a small fix 😃

should be "returns a"

Just a small fix

Maybe can paraphrase to "sorts the list of tasks according to the deadline of the task. Tasks with deadlines approaching soon are displayed at the top while tasks with no deadlines are displayed at the bottom"

maybe next iterations we can avoid null but looks good for now!

good catch haha

maybe can save the condition in a boolean to make it clearer instead of having the comment? just a small suggestion

was gonna say the same but yes lets discuss

nice!

can we use enum? just a suggestion

just a small issue.. can be a bit ambiguous (name can be interpreted as name of task / name of module) so can be more specific. looks good otherwise

should follow the given format e.g. deleteTag 1 t/tagName

agreed

listOfModules (should be plural). Small issue though

idk if extra lines in between if blocks are adhering to 2103t standards.

not sure if its better to mention the return like this or use '@return' instead. Just a small issue

Looking at the other parse methods in the various commands like add, ArgumentMultiMap and tokenize is used instead of this way of split. Although valid, maybe for consistency sake can use that?

im not sure if trimming is necessary here as in ModuleBookParser's parse command, userInput.trim() is already called with the matcher. Good to double check there haha

Yes i agree with @QY-H00 here. Was going to mention that highly likely law of demeter is violated here. Although, if u think the violation is justified, its fine (according to prof, can justfiy for convenience sake at times). If not, i think its better to add a method in!

think there is too much method chaining going on so may be good to change this implementation? Up to u if you think otherwise.

I think the naming of the variables can be clearer here!

do you think javadocs required here? since they are public methods?

unecessary else block?

nice catch!

I see that u have an array of valid modules in ur ModuleManager class... I think it will be good if we specify the valid modules in the UG.. if not, it might be reported as a bug later on haha

referring to this in the comment above!

shouldn't each assignment be on the same line?

hmm correct me if im wrong, but youll be adding the raw workload of each task to the module? Maybe, as a better indicator for modules, we can calculate the proportion / fraction of the total workload for each module? I think that wll be a more useful number for the user? Maybe the rest can pitch in here too

ok will do

done

closed. Didn't run CI

Done. Waiting to merge in master DG after review

done

no changes for storage. LGTM!

Overall LGTM. Just added some comments pertaining to previous ongoing convos.

LGTM

Agreed

wah this is complicated, do you want to split it one by one before return

A complicated return also

tks for this change

You forgot to include the project-index in this message_usage

As Samueal commented on my PR before, the getZeroBased method alr checks for negative value, so this is unnecessary

tks for the change

I'm not sure about this 2, one you taken from address field right, and the original I take from project name.

I think for the rest of the pr, it's ok

hey samuel I'm not sure about this, but as I see it from javadoc format of other methods, there should be a blank line between description of method and parameter, return, throw

I think this is a complicated boolean expression as code quality says about it. You can choose to keep that or separate them 1 by 1

should this have a javadoc

this 2 method missing javadoc also, as for other builder, you put javadoc for constructor also

Ahhh, OK I think that is all I can spot for now. For cases to test, I cannot think of other cases to add.

add a blank line in javadoc

leave a blank line here too

here also

can we use requirenonnull for this

0 here sounds a little bit like a magic number here, should you put another variable for it?

1 here also

Here are the variable for it right. you can mention it if possible

leave blank line

can we use !requrireNonNull here

I think for the rest I agree

I think this should be userInputInvalidProjectIndex

This should be userInputInvalidTodoIndex also

I think here should rename also so that it refers to index of project not project.

This one's javadoc should have a return statement

This one also

add blank line for javadoc

here also

yeah i think its weird yesterday too, but not sure thanks for the comment

@vevek Am i addressing your full name correctly

resolved

ah for this actually the invalid format is thrown above

the parseIndex has its own exception throw for invalid Index. So no problem with this, and actually I change it because if invalid index, the correct exception should be thrown (invalid index), not (invalid fomat)

resolved

Closes #22

Great work 😃 . Can I also suggest adding the role "Developer" to everyone. I feel that it would help reflect our roles clearly.

Agreed

Cannot merge because of irrelevance to the project

closes #5

Omg i just did it again. So sorry @vevek 😃))

closes #49

i have implemented the addEto command. Please give some input. For the codecov test, that might be because of missing tests. For now, you guys can comment on any logic faults in my code. I will try my best to complete the documentation and the tests for this command asap.

And also, i have tried out the code but seems like only the command result shown up. Is it because of the ui not ready yet? Or maybe I don't know how to make that shown up. Please let me know.

This issue no longer fits the project as the idea of project changed. Closed for ease of distraction.

I think it would be better if you could sort the user stories by priority.

I think the numbering for this extension should be 1a1.

I think this use case should resume at step 3 as the list would still be shown even after the error message, so ClientBook does not have to show it again.

A suggestion would be "2a. The list of matched clients is empty."

Same here. A suggestion would be "2a. The list of matched clients is empty."

Would be nice if can add a screenshot of how the feature works.

Would it be possible for the success message to output the attribute that was specified as well? Like "Listed all clients with address attribute as filter."

A suggestion will be to do "if (!isAttributeSpecified()) {".

One suggestion would be to use switch statements.

Same here. Suggestion to use switch statements.

Suggestion to name this isFirst to isFirstAttribute to make it more understandable.

Same here for the isFirst boolean as mentioned above.

I think there is a typo here. Should be "...client management tasks faster than traditional...".

Would it be better to define what is "home folder"?

Instead of "jar file" maybe you can put "clientbook.jar" with a markup to be more specific?

Adds a client named John Doe along with his details to ClientBook.

Index has to be within the range of the list as well.

Maybe can include an image for the "list -policy" command, since the "list -phone -policy" one already has one.

Also can add "An optional attribute option can be added to show the list of matched clients with only the specified attribute."

The image format is different from the other images (e.g. the title bar not shown).

I think it is okay to remove the OR search part because I think the user might not understand what it is.

Same here, about the index being in range of the list.

I am not sure if these 2 lines should be in this policy command section. Feels like it should be stated in the add/edit command section where the policies are added/edited.

Same here with the index within range.

Will be good if can describe how the sort by policy command displays the list. Maybe add an example.

I think there is a typo with "exclamation".

How about "Copy the file to the folder where you want to store the ClientBook application and your client information."?

Yeah, this is much clearer!

Yeap, that works too!

Like a screenshot of what happens when you perform "sort -i -dsc".

LGTM 👍🏻

Should the command be find instead of search?

Would it be better to name the field expiryDate to date? I think it would not be as ambiguous and would be easier to understand.

Instead of first checking if the expiry date format is correct using regex, he parses the input right away and catches DateTimeParseException to determine if the specified input is a valid expiry date. I think it will still work as intended.

Should this nonNull check for expiryDate be removed?

Should this be "Sorts the items from the inventory" instead?

Would it be clearer if we state "sort quantity will display the items in the inventory in ascending order of quantity."?

I think we can provide example usage similar to how the other commands provide it currently as well.

I think it is fine to leave it as it is as this is just a custom comparator. Perhaps, the naming of the class can be better.

Would this message usage be misleading and give users the idea that both tag and location can be specified? Would it be better if the message usage is [l/LOCATION]/[t/TAG]?

Would it be better if it explicitly states "user-specified number of days from the current date..."?

Perhaps, you can try checking both conditions for trimmedArgs and numOfArgs in one if block and throw a ParseException to avoid repetition?

I'm assuming this is not case-sensitive. Does it mean that "Weeks" would not match "weeks"? If that is the case, we have to clearly specify this in the user guide.

I think this way of checking would break if the user keys in "reminder 3" with 2 spaces instead of 1. This way " 3" your argument would be " 3" which would cause a number format exception which would be caught and eventually be thrown as a parse exception. Are we going to be strict about this? Currently, the other commands don't seem to take into account additional spaces in between prefixes and command word.

Can you clarify the way you are parsing?

I agree with Jay as well. I think it would be less ambiguous and would be standardised across the commands.

Does this violate the law of demeter since you are getting an object from item and accessing its field?

I believe Item should provide a method that retrieves expiryDate directly.

AB3 used address book and it's name interchangeably. In our case, our UG uses StoreMando and inventory as the two terms to refer to the app. Would it be better if you change all the usages of address/location book to inventory instead?

As mentioned above, this could be "causing another modified inventory state to be saved into the storeMandoStateList."

My personal opinion is that the welcome message should just be "Welcome to StoreMando!". Just a suggestion to consider.

I agree with Kumaran. I think it should be written as "User searches for items that are expiring within the next 3 days".

I think this should be written as "User inputs a negative number".

Perhaps, I think this step should be removed.

I think can it would be better to phrase it "User requests to delete all items in a specific location".

I think this should be "prompts the user for a correct location".

Would the benefit be clearer if we specify "I want to clear all the items in my room at once so that I do not have to waste time manually deleting all the items in my room"?

Should this be "StoreMando prompts the user for the correct input"?

Can we abstract it out further? This method calls setItems method of model class and retrieves model's own attribute to pass back as a parameter. I think we can create a method to abstract this out further.

Can you clarify how is the parsing is handled? Would this break if there is an additional space passed in?

Fixed

Fixed.

fixed.

Agreed with Wei Hao. I think it is fine to leave it as it is since there is no fixed input format and prefixes can be specified in any order.

Fixed.

Thanks for the suggestion. Fixed.

This follows the original setup. I think it's fine to leave it as it is for now.

We can leave it as it is for now as mentioned above.

Value was used to standardize across all the fields an item model is composed of. I think it would be more consistent if we leave it this way.

I think we can display the warning in red color in our GUI instead of using all caps to warn. I feel it would be friendlier that way.

Fixed.

I think it is still fine as the class MainWindow has helpwindow as the immediate neighbour and I'm passing the URL as an argument to the displayWebsite method in BrowserUtil.

Thanks for the suggestion. Fixed!

Upon further thinking, I have shifted it as per your suggestion as it was more practical. Thanks and fixed!

Alright, fixed!

I think both are fine but I'll change it to make it clearer.

The anchor tag will not be displayed and only the hyperlinked quick start will be displayed.

I have updated the features section header to contain the anchor tag. Thanks for pointing it out!

Yes. This has been included in the existing user guide and I did not make any changes to it. Therefore, it is not highlighted in this pull request as a change.

Fixed!

I think it is better to be as explicit as possible in the user guide and there is no harm in doing so.

As mentioned above, I think it is fine to leave it as it is. Perhaps, we can get the opinion of others as well.

The subheaders will appear in bold when displayed in HTML so it will still be fine.

should be [-t TAG...]

same here, should be [-t TAG...]

and im not sure whats that special char. but if its intentional then no problem!

Cancel, not a typo

For the ... , are multiple tags searches allowed? Doesnt seem to be allowed atm

rename to sortPersonList since not the filtered list being sorted

suggestion:

This one our userguide currently is "tags" too but would "tag" be better?


    public static final String MESSAGE_USAGE = COMMAND_WORD + ": Lists all tags and the count of persons tagged in the Hippocampus. "

Prefer like that haha but this one up to you


        model.getAddressBook()

                .getPersonList()

                .forEach(p -&gt; p.getTags()

                        .forEach(t -&gt; tags.compute(t, (k, v) -&gt; v == null ? 1 : v + 1)));


        boolean noNameAndTag = noName && noTag;

want to use .contains for search tag too?

just to standardise with other parser classes if you want haha


        ArgumentMultimap argMultimap = ArgumentTokenizer.tokenize(args, PREFIX_FIND);

        

        Optional<String> keyword = argMultimap.getValue(PREFIX_FIND);

        

        if (keyword.isEmpty() && argMultimap.getPreamble().isEmpty()) {

            return new TagsCommand(tag -&gt; true);

        }

Add new line


            + "person tagged in Hippocampus.\n"

Swap error message


            } catch (DateTimeException err) { // date in wrong format

                throw new IllegalValueException((Birthday.MESSAGE_CONSTRAINTS));

            } catch (IllegalArgumentException err) { // birthday year exceeds current year

                throw new IllegalValueException(Birthday.MESSAGE_YEAR_CONSTRAINTS);

IllegalValueException was correct haha

IllegalValueException is the one caught by storage. Or else an invalid date in the save file will not be handled and program will terminate


                throw new IllegalValueException(Birthday.MESSAGE_CONSTRAINTS);

I think she was following the wording of constraint for address field. Do yall think it would be good to change that too? or we can leave both too

I believe the == check here was intentional cos it is checking the exact object (the static string).

Otherwise LGTM

For marking event as done (in the event branch)

I simply used list.contains() since that uses .equals() to check.

May be slightly less efficient time complexity wise, but could be more readable. Which do yall prefer?

If prefer this then LGTM, i can update the other


    private DeleteContactCommand createDeleteContactCommand(String args) throws ParseException {

        String[] strIndexes = args.split("\\s+");

        List<Index> indexes = new ArrayList<>();



        for (String s : strIndexes) {

            Index index = ParserUtil.parseIndex(s);

            if (!indexes.contains(index)) {

                indexes.add(index);

            }

        }



        return new DeleteContactCommand(indexes);

Here can just leave it as DATE.

Since to the user, event date is basically a date (unlike birthday where they are extra restriction).

Or if decide to keep, put it as one word EVENTDATE, or else it seems like 2 arguments


            + "[" + PREFIX_DATE + " DATE] "

Might have missed out this? Not sure if it affects other stuff, but after i changed it looked better haha


.list-view {

    -fx-background-insets: 0;

    -fx-padding: 0;

    -fx-background-color: derive(rgb(243, 229, 231), -10%);

    -fx-border-color: white;

    -fx-border-width: 1;

}

Alternative to probably make it simpler and slightly more efficient


    private void checkIfHasTags(Person personToCheck) {

        Set<Tag> personTags = personToCheck.getTags();



        for (Tag targetTag : targetTags) {

            if (personTags.contains(targetTag)) {

                editedPersons.add(personToCheck);

                return;

            }

        }

    }


Toggles between Dark and Pastel theme


     * Constructs a {@code GuiSettings} with the default height, width, position, and theme.


     * Constructs a {@code GuiSettings} with the specified height, width, position, and theme


    public static final String MESSAGE_USAGE = COMMAND_WORD + ": Toggles PartyPlanet's theme between Dark and Pastel.";


            commandResult = new CommandResult(String.format(MESSAGE_SUCCESS, "Pastel"), false, Theme.PASTEL);

Optional: can be private?


    private void setThemePastel() {

Optional: can be private?


    private void setThemeDark() {

Missed out in initial suggestions


 * Toggles the theme of PartyPlanet between Dark and Pastel.

Maybe is just the phrasing confusing. So in the latest commit I phrased the main info as

If provided with tags
- Delete every person who is tagged with the specified tag.
- If the person is tagged with another tag, only the specified tag will be removed. The contact will not be deleted.

Then for these examples, I left it as

delete -t colleague -t cs2103 deletes contacts with tag "colleague" and contacts with tag "cs2103"

Cos the happy path is to delete the contact. Then ONLY IF it is tagged with another tag then the contact is not deleted.

Did not change, discussion is below

Edited but phrased a little differently, check latest commit


    public static final String MESSAGE_DELETE_PERSON_SUCCESS = "Deleted the following persons: %1$s";

Yup makes sense!

Edited, info in comment below

LGTM

Also update userguide to include this command

Yup I considered this but thought it was too inefficient to scan through all whole list to see if all tags specified exists.

Perhaps I can add some kind of static set in Tags to keep track of all tags that currently exists

I implemented a similar feature in my IP so I can probably PR later and see if yall like it

Actually, since year not really important for a birthday, maybe we can store it in a MonthDay object instead. Then no need request year input at all.

Perhaps can also change display of bday in a form of "11 Dec 99" instead of the current 1999-12-11.

Alternatively, idea to put the logo as a background. Altho tbh I dont have much idea how to do it haha

Added a min height so for aesthetic purposes

Is partial search not going to be the default?

Yup that might be better. Since we implemented partial in the first place cos it would be more commonly used.

Even if decide to keep it as exact by default, i think would be good to minimally make it non case-sensitive.

I think we agreed that memory not really a concern atm since undo history doesnt carry over across sessions.

But i think no harm adding later on!

Yea I dont mind. I feel like even ~10-15 undo is probably more than enough.

But i think wait til event branch is integrated with the history too

Throwing error is one way, and the ~easy way out~ other way would be to simply document this _behavior feature (i.e. only the last sort prefix given is used), just like for the add command. Which do you prefer?

"If a parameter is expected only once in the command, but you specified it multiple times, only the last occurrence of the parameter will be taken.

e.g. if you specify -p 12341234 -p 56785678, only -p 56785678 will be taken"

Yup our UG already states this case. So alls good!

Implemented --exact and --any as suggested above

delete -t a -t b deletes any contact that has tag a or b

delete --any -t a -t b same as no flag

delete --exact -t a -t b only delete contact with BOTH tag a and b

Currently contains bug stated in #162 . Will follow the solution of list

closed as branched out from wrong branch

Split checkToBeDeleted() into

containsExactTags() when --exact flag used
containsAnyTags() otherwise.

Methods above no longer modifiesdeletedPersons list. Addition to deletePersons carried out in execute()

not a bug

Final functionality as follows

Before: delete -t TAG [-t TAG] was "delete all contact with specified tag, unless they have other tags"

Now: delete -t TAG [-t TAG] delete contacts (in filtered list) that has the provided tag

delete -t a -t b deletes anyone who has both tag a AND tag b, and display persons deleted

If no one deleted, return message These tags do not exist in persons listed. No person removed.

Flag --any

If this flag is specified,

delete --any -t a -t b deletes anyone who has EITHER tag a OR tag b, and display persons deleted

If no one deleted, return message These combination of tags do not exist in persons listed. No person removed.

extra space before />

Is the space intended to be there?

Missing space

Why call it ProjectsFolder instead of something like ProjectList?

Extra quotation marks?

here can perhaps use Project.addEvent()? since it does not seem to be used anywhere

This is not a problem with the test but I think we should put project name instead of project is the message?

Perhaps we can change all occurences of Index.fromOneBased(1) to INDEX_FIRST in this file?

To be consistent, 1 should not be bold

Perhaps it is better to put this under a level 3 heading named others or something? It looks a bit strange to me to have a level 4 heading parallel with level 3 headings.

According to seedu md standard, we should put an empty line after each heading:

same here

Should this be Contact List instead since we have only one such list?

Yes! That would be nice!

My bad, forgot to change this line after copying from the sample.

@samuelfangjw Changed as requested!

General hashcode contract states that

When hashcode is called repeatedly on an object, should return same result unless object has been modified.

Two objects that are equal (according to equals method) should return the same hashcode

Different objects do not have to return the same hashcode, although it's good if it does so.

Wah I never knew there is such a contract. Will do it tomorrow!

For the third point I think it should be the other way round. Objects that are not equal should return different hashcodes as far as possible.

I think it should still be pretty safe to check for different hashcodes for different objects since it should be quite unlikely to collide. Do you think we should remove this check?

maybe block letters? follow the format of the other fields

can consider removing the extra blank line?

I think this JavaDoc comment can be further refined.

Can I check why you think it is better to not differentiate code/command line keywords from normal words in a sentence?

Would it be better to display all the possible parameters?

Do you think it would be better to give the user instructions on how to initialise/use our application? Or are there reasons why you think this should not be here?

Could you check if there is an & delimiter for the subsequent keywords?

Would it be clearer to the user if we used markdown to format the keywords here too?

Do you think a person should have a tag describing a policy if s/he has no policy number?

Do you think it would be more consistent if you ended this line with a fullstop, just like the above lines?

Do you think we can use more suitable tags given the insurance agent-client relationship?

Would it be more consistent if the policy number format followed that from above? It is not very consistent with the policy number formats written in the Quick Start section, and the Features > Add section (there are 2 different formats in 3 different sections).

Also, there is a spelling mistake for the t/premiunplan. Do be more careful next time!

Do you think it would be good to clarify on the maximum number of flags that can be used in each command execution?

As Yong Shen has suggested in my PR, do you think it would be be more intuitive to the user if we rename policies as insurance policies instead, so that the 'i' flag is more suitable?

I was wondering if it would be good to clarify if a keyword is bounded by '&' delimiters (if any)? Else, some users might think that keywords are words and mistake a search for 'Hans Bo' to return the same as 'Hans & Bo'.

As in the above comment, perhaps we can add in an example where we should find alex david to show the difference between the 2 types.

Just something minor: would be good if in our documentation we can clarify that our search is case insensitive.

Would it be clearer if the regex was stored as a constant so that another developer would better understand what you are trying to split by?

I realised that this List>String> is used in all the subclasses. Perhaps we can consider shifting it to the parent class in the future?

Just to check, this test checks if the address contains "12345", "alice@gmail.com" or "Alice" right? If so, perhaps it would be better to clarify that the person's address does not contain any of these keywords and thus the match fails? Else, it seems like we are testing for all the fields mentioned.

Same comment as the other file!

I like that this is so succinct, but others who are less proficient/new to Java might find this hard to digest, since it condenses a few steps into one. Do you want to consider writingn this on separate lines?

Do you think it would be more readable if these two strings are abstracted to static final variables?

Do you think it would be more appropriate to name the test to describe that you are testing reapeated unlocks/mutability rather than just "equals"?

Would it be more appropriate to name this variable POLICY_ID_CONTAINS_ANGULAR_BRACKETS rather than POLICY_ID_CONTAINS_CARROT? Since carrot can refer to ^ too.

I was thinking that it would be better to link to the User Guide itself. Perhaps I should change the description of the URL. Or would it be better to directly link to Quick Start?

Great suggestion! Thanks!

I was thinking if I name it getPolicyUrl, another developer might mistake it to return a String rather than an Optional. What do you think?

Good point. I've changed it!

Good point, I didn't notice that.

Each new line is meant to separate the logic from the other lines, so that each "section" gives me 1 variable/object that I would eventually pass into the assert function. I felt that it looks cleaner that way. What do you think?

Okay, thank you!

How should we define it actually? I was thinking the same thing, but couldn't think of how to define it properly.

Okay, changed!

Okay, I changed the positive integer line to the following:

The index must be more than 1, and less than or equal to the index of the last item in the displayed list.

Is this clearer?

Okay, added!

Okay, added.

Hmm okay, I think I will be adding a section to talk about the fields that can be added for each contact.

Thanks for spotting this!

Can you clarify what you mean by this?

Okay yes, thanks!

FLAG uses the identifier of each attribute to help the program identify which attribute you are referring to.
For more information about the identifiers for each attribute, refer to What information can we store for each client contact?.

Added the above 2 statements, hope they make things clearer.

Okay, I've added the below 2 statements for clarity:

The data file is stored in a zip file inside the data folder in the same folder.
If you previously set a lock for ClientBook, the zip folder can be unzipped with that same password.

I will be doing the test for the new command in the next iteration.

I think we should keep Mainstream OS in the glossary as mentioned in our NFRs

Perhaps it would be more consistent if we used 8 spaces here for indentation as well.

Should we revert this back to 8?

Perhaps we could make dd-MM-yyyy a constant string, since it is also used in line 40

Same here for HHmm

Indentation should be 8 spaces

Same here indentation issue

Indentation issue as well

Same here for lines 132, 133 and 136

Duplicate opacity styles

Duplicate opacity styles here as well

Indentation issues on line 22 and 24

Not too sure about this, but should this be private static final?

Let's standardise to double quotes for all strings

Should be okay to remove the old getMeetingList() and getDateList() which returns empty lists right?

I think this can be removed?

I think we should add spaces around the ->

Extra empty line here

I think BIRTHDAY is fine here, or perhaps DATE instead of DATETIME

Could we add a "*" multiplicity to Event? Might be good to use labels here as well since there are 2 links to Event: dates and meetings

Would be good to standardise the term css to be always in lowercase

Do add a space between the hashes and the title itself, as I'm not sure if jekyll can parse the markdown properly without

Add a * before the example here for consistency with the rest of the UG

Would be better if we used the theme's colours here

Hmmm the idea is that it could be used for any kind of special annual date, not just anniversaries. Any other ideas?

We can't simply compare LocalDates as we're not taking year into account here, since the events are repeated annually.

I'm unable to put them in their respective classes because it requires the Person's name, which is not stored on the Birthday and Event class.

yeah its just a label now no styling yet

I realised in some places I named it UpcomingDates while in others they were UpcomingEvents. Actually, I just realised in logic it's still called UpcomingDates. I think I'll just revert and switch it all to UpcomingDates.

@Assyarul Has this been resolved in #58? I think we can close this if so.

I think there might be some settings in your editor that are automatically modifying whitespaces/newlines of the code because this is actually a tutorial file.

Same as the above comment, awkward whitespaces in the method header comments.

Can consider removing this since we are not checking for duplicate endpoints or it might eventually get flagged as dead code.

Find command was only applied to names in AB3 but ours is likely to include all fields in the search. The test here may be referenced for implementing tests for our version of the find command.

Might want to be careful of this. Our application might not need this additional check as even having the same method/url may not mean they are the same if the data being sent is different (i.e. data in request body in a POST request)

To add on, I think it's possible to check for duplicates but in this case every single field (including tags) would need to match (unlike just name and address in the original AB3).

I think the check has been performed in isValidMethod below 😄

I think there's a good chance more tests will be breaking given how the header and data attributes have not been implemented yet. At the same time, I am also hesitant about commenting out all the test cases.

The consideration I have is that making several small fixes to pass them might be easier compared to having to make a single big change to all test cases at the same time. Depending on the implementation for header and data, ensuring the test cases pass now may prevent further complications down the road. Just a thought 😄

Should there be a newline here?

Might be good to populate the sample data in the correct format even though currently they are just stored as string when entered e.g for data it would be '{"key": "value"}'

Just a small point but the comments are not updated here.

Maybe instead of listing find command twice can differentiate the 2 ways to use it while listing it just once because on first glance it looked like a duplicate (similar to how run was listed).

Might help to mention that this is the second format since the previous one said 2 formats.

I think the match for ftp and file are not necessary since they are not protocols we support.

Minor typo here 😛

Need to update the links here 😄

Need to replace instances of AddressBook and Person 😄

Minor thing but "an Address" might sound better :3

Links here need to be fixed.

Should the Person reference be replaced with Endpoint instead of Method?

Looks much cleaner now! 😄

Minor typo for endpoint here.

Same typo as above.

Should it be find command instead?

Using an add might sound more correct here.

Just to leave an update here:

The team has had an internal discussion and reviews will be left open to all members of the team (except the PR author) for now. This is slated to change as the project grows and the codebase becomes more complicated.

I actually considered this! Then I saw that the other files were not doing it this way so I thought to just follow what's already there as closely as I could :3

This was required to address the error raised: not on FX application thread; currentThread = Thread-4

I agree the animation should be further improved down the line 😄

Thank you for catching that, it's fixed! 😃

There's no check for header to be in json format. As an example, the header will be added in this format: -h "Content-Type: application/json". That said, when retrieving the individual key and value, I noticed that this was what I got: ["Content-Type: application/json"]. Hence, I trimmed the brackets and quotations to obtain the key and value. Ideally, the headers should be processed before being stored but until then this will still work 😄

Great suggestion, changed this in the latest commit! 😃

Reduce the spacing between the title (guide) and the subtitle (version number) xD

Great photo! @tjtanjin By the way, there is this slight distortion on the center-right edge of the photo, not sure if you have noticed it. If you don't mind I will go ahead and approve it.

On close inspection, I realized that you added this [[homepage](https://tjtanjin.com/)] in front of the Github and portfolio links. While I have no issues with the inclusion, I wonder if moving it to the back of GitHub and porfolio will make it more standardized? This is because not everyone might be adding their homepage link. What do you think?

I like the suggestion for shifting homepage link to the back, have updated it accordingly 😄 With regards to the image I will source for a better one at a future date but this should suffice for now 😅 Thanks!

Resolved with #75

@tlylt Thank you for taking the time to scour through the changes 😄 It is indeed a lot of updates at once and I should have provided more context in the PR message. Regarding the points you raised, here are some of the missing context (which have now been included in the original PR message as well):

The refactoring was done partly through IntelliJ and partly by hand, and during the refactoring by IntelliJ, certain imports were automatically changed (such as the one involving Messages and CollectionUtil that you mentioned). I have previously combed the files in general but a few of these change in imports were not caught (though the latest commit should have caught all if not most of them).
The change of seedu.address to seedu.us.among was a result of a brief discussion but it has not been cast in stone, so please feel free to suggest alternatives to the package names 😄
Finally, this PR changes only names and terminologies with no update to the logic at all so test cases would still need a separate update eventually :>

Hopefully these help provide more context 😄 I'll proceed to merge the PR for now and we can do a future PR to rectify stuffs if necessary, thank you!

Closed as checks failed to run when github actions went down earlier.

Resolved by #95

The image seems broken, could you double-check?

Thank you for pointing that out 😄 It's fixed now 😄

Addresses #112

Addressed by #111

The SendRequest class perhaps can be more aptly named as Request? Given that we are indeed modeling a thing that is called Request and the word Send seems redundant.

Just my 2 cents, other than that LGTM

Ok I renamed the logic classes under endpoint and renamed the execute method to send as well which sounds more apt: new GetRequest(endpointToSend).send();

Merging it since the changes are small 😄

@tlylt I tinkered around with this as well. Unsure if there is any lag reduction but I imagine we can play around with the initial text to set in the box to mask this from the users. The counter is just a placeholder, I will likely replace it with Processing Command... (the dots will change between having 1 to 3 dots to reflect progress). Just 2 key points I wish to bring up:

Firstly, although it works, the current code implementation feels rather messy because I had to wrap Platform.runLater a good 4-5 times around codes. If you had a better way to implement this, I'd love to hear it. If it helps, the reasoning behind my many uses of Platform.runLater is to deal with an error saying not on FX application thread; currentThread = Thread-4
Secondly, I did the UI threading within the MainWindow#executeCommand but this would mean that sometimes, even the fast commands like add and remove will see a quick flash of the counter (the pseudo progress indicator currently). Resolving this within MainWindow would look very messy as well. I am also still looking for a way to improve this.

I will tidy up and abstract parts of the code as much as I can tomorrow and try my best to make a PR before the team meeting so that we can further discuss this.

Great job, while you are at it, could you reduce the 5-second delay that emulates the wait time of an API response 🥺

Curious:

how did you fix the lag?

if we are disabling the user's ability to input while running the request, wondering if it will be better if we still show what he entered in the command box and clear it after the request is done? Not sure if this is the case already... for discussion only

I'll make a PR really soon to remove the wait time :3
Shifting Platform.runLater above the sleep code removed the lag (which came from the initial 1 second sleep)
Yup the entered command stays in the command box when frozen 😄

Not sure if I am getting this right, the request is cast to HttpPost for all the requests?

The setEntity method is unable to be called on HttpUriRequest and hence the casting. Whether this casting will be universal will be looked at again when more methods are added. If there is a need to then further abstraction needs to be done here 😄

Resolved in #145

Note:

User header input needs to be enclosed in quotations for example: -h "Content-Type: application/json".
Need to remove above mentioned quotations before storing.

Important:

The second change directly affects the parseHeaders method within HeaderUtil.java. When updating this, change the following line:
- Before:

headerString = headerString.substring(2, headerString.length() - 2);

After:

headerString = headerString.substring(1, headerString.length() - 1);

I think this is a great suggestion, since v1.2 is being wrapped up let's do it in v1.2b 😄

Addressed ever since Person was morphed into Endpoint.

Addressed in #125

interface as in the screen?

I think this is referring to the endpoint list on the left panel.

Sample endpoint 3 periodically returns an empty result with the following error:

"JavaFX Application Thread" java.lang.StackOverflowError

There is no known way for the user to directly cause it as of the time of writing this. It seems completely random. What is worth pointing out is that sample endpoint 3 returns a very long json result which is also reflected in the extra 1 second it takes to load the result into display.

This seems to be a performance issue and we should consider how to address this as soon as possible.

Have tried a few times and wasn't able trigger it, if it is a peculiarity with regards to this API, perhaps we should remove it as of now.

I think it's alright we can leave it there and hopefully it will appear again (happened twice to me so far) so that we can look at it. I am thinking to catch the stackoverflow error but not sure if that's actually legitimate.

Would this compromise the command line friendliness since it involves using the mouse to hover?

Might have a potential fix to this. Will look at using appendText in ResultDisplay and update again.

Another update, I tried both appendText as well as ListView instead of TextArea in our ResultDisplay. In all cases, the lag still features prominently and because the bug is not deterministic, I am also unable to tell if either of them actually fixes anything. With all that said, I have come across quite a number of threads discussing about the performance of JavaFX and in particular, it seems TextArea is not good for very lengthy strings. In fact digging deeper there's quite a lot of criticisms for JavaFX performance. With all that said, I will likely opt to catch the stackoverflow exception.

Note: I cannot tag prof damith here but I'll find time to clarify if this can actually be considered a bug of ours. If truly it is because JavaFX is unable to support such lengthy input without crashing, then hopefully it might be considered an inherited bug.

If the length of response is a concern, perhaps we can check the response length and limit that to a reasonable number such that we display a proper error message when the length exceeds that.

My concern is that this might be considered a bug as if we consider the fact that our application causes "loss of data" when calling an endpoint. Will really need to clarify soon.

Yup this is not high priority so we can try this out and discuss some time later.

Ok nvm I misinterpreted this. List command does seem useless given how our list is always showing, but without list, if you do a find command and get a subset of endpoints you have no way of going back to seeing all endpoints again. This is unless find command supports a wildcard search or defaults to showing all endpoints if no arguments are given but at this point it seems easiest to just keep list.

Closed in #232

Resolved in #166

Possible duplicate of #148

Looks good! As discussed, it would be even better if the background image scales with the screen size as well 😃

This behavior is also seen in CURL so I think it will be a good enhancement 😃

@tlylt @ong6 Is this discussion still ongoing or can it be closed?

Currently if a field is blank it states No Data, does that resolve this issue? @ong6

Closed by #259

I did a quick skim of the files and it seems that the whitespace removed from the likes of PREFIX_METHOD are instead being introduced in the strings they are being concatenated with. Am I overlooking anything?

LGTM!

Do you want to also change the example commands at the top and the command summary at the bottom? If not, I can do it 😃

Thank you for catching those, I have updated them with a commit 😄

I did a quick skim of the files and it seems that the whitespace removed from the likes of PREFIX_METHOD are instead being introduced in the strings they are being concatenated with. Am I overlooking anything?

Hi the only thing in the PR that affects code is the prefixs being changed in CliSyntax.java. For the add, edit and run, I just made the error command messages more readable, for example: from

"add -xget -usomeurl -hsomeheader" to

"add -x get -u someurl -h someheader"

Okie @ong6 introduced the whitespace in PREFIX_METHOD in this commit 5 days ago so I think's best for him to do the review.

@ong6 May I clarify on what the updates for the above commands are in the user guide? I missed this and just merged a user guide update :X

Resolved with the function to add endpoint and send them again simply by calling an index.

Resolved with the current result display showing API responses.

Resolved with the application currently having a very simple design and clear information for endpoints.

Resolved with the functionality of send command.

Resolved with the API responses being returned in result display and being stored in imposter.json.

Resolved with the current capabilities of the application to allow easy testing of APIs.

Resolved with the current API calls returning a response time.

Resolved with the support for tagging of endpoints.

Resolved with the show command displaying responses from the last run of an API call.

@tjtanjin the run and send test lowly unreliable they sometimes pass sometimes fail on my comp and I couldn't even commit my work on sourcetree.

Strange if it's inconsistent, if it happens again can create an issue for it and then paste screenshots of the error messages easier to debug 😄

Resolved since all features are now supported conveniently through command line or keyboard shortcuts.

Resolved in #280

Resolved with the enhanced find and tagging functionality for endpoints.

Duplicate of #8

@AY2021S2-CS2103T-T12-4/developers Just a note that this also remains an important issue with regards to the aspect of UI refinements for v1.3.

I think it's not stated obviously but the background is suppose to differ from theme to theme and both dark and light theme has a plain background currently 😅 To clarify the bug for light theme is obvious once you only have like one endpoint left in the left panel.

@tlylt Very detailed help prompt, looks ready to be merged! 😃

The endpoint associated with this error returns a response body of length above 800000 and infrequently crashes in JavaFX when setText is called. There seems to be no better way to improve performance for this in JavaFX (have tried ListView, staggering the setting of text etc). As this lies with performance issues in JavaFX, it is more practical to address this in our requirements where we "cap" the length of response body for a reasonable performance of the application to be less than 100000.

Thanks and sorry for the conflicts!!!!

All good xD Thanks 😄

Refer to latest issue at #310

Resolved with the now more detailed error messages.

Seems similar to what @ong6 suggested in issue #312 except his was for the clear command. Might want to entertain possibilities of solving both these issues at once.

Would it be possible to link directly to the quick start section as well?

Do you think "clients' contact details" will be more suitable?

Would it be better to have a static constant for each direction?

Would it be better to have static constant for each direction?

Is using "sorts" instead of "sort" better as it follows the convention used by the other commands?

Do you think that we could use 'insurance policy' instead of 'policy' because it would be more intuitive for the i/ tag that we are using? At least for this section. It seems fine when we are talking about the policy command.

Maybe you forgot to change some information after copying them from find command.

Is there a use for this logger?

Just to clarify Policy_ means even if different insurance companies have different ways to label their policies, they should all be labeled starting with Policy_? E.g. company A labels policies P#12345, company B labels policies Life#7900, the respective policy ids in ClientBook would be Policy_P#12345 and Policy_Life#75900.

Would it be better to name the method getPolicyUrl? The if-present characteristic of it is represented by the fact that Optional is used.

Perhaps a grammatical error here? Is there a reason why the original comment was changed?

Would it be good to also update the user guide to reflect this required format?

associated with a ~~selected~~ client?

Maybe "delete client contact" instead, to be consistent with add and edit.

"hence" sounds redundant when the line is read.

Could use INDEX instead of "The index", to clearly indicate that you are talking about INDEX in the command.

This line suggests that only 1 optional attribute can be added, while one of the example below shows multiple optional attributes being used. This can be confusing to the reader.

Did not state explicitly what the flags are used for. The usage is clear to us as we are the developers.

Also, would it be better to list the flags in a table? Idk

A suggestion would be "The delimiter & between keywords allows you to search for Clients using multiple keywords."

Can draw similarities with list function. E.g. "Similar to the list command, optional attributes can be added to show only certain attributes in the search result"

Aside from the example on sorting by name, do you think an example on sorting insurance policy is needed?

Maybe "After setting a password, the application" or ClientBook instead of application.

ClientBook's ~~ClientBook~~ password

ClientBook's password is removed.
Suggestion: Future launches of ClientBook will not require a password?

Currently the json is stored in a zip file regardless of whether it is locked. If its locked then there's a password to the zip file.

Done

Have changed Tag into InsurancePolicy @ line 24. Will be keeping the JavaDoc comment to follow the JsonAdaptedTag class from which I adapted the code.

Appreciate the feedback! However that was the format that came with the incumbent code e.g. DeleteCommand's equals().

Yes, the original intent is for unlock command to work even without CURRENT_PASSWORD if ClientBook is not locked. However, in hindsight this is confusing for the user and I have changed CURRENT_PASSWORD to be a compulsory argument in the UG.

That is a great suggestion actually.

One possible way would be to encrypt a password file using a secret key within our code which when the program runs, will be decrypted and stored in program state and encrypted immediately.

The user can then enter lock without any parameters to lock using the old password.

However, I do feel that the effort to return ratio is rather minimal as it does not make our program any more tailored for our user than what already exists.

Good point there, I was looking at the same API a few days ago too. The methods ZipFile.isEncrypted() and ZipFile.extractAll() did not specify which type of ZipException was thrown. The try blocks are separated because I was using the second exception to return false, while the first try block was used to catch the error thrown when checking if the zip file is encrypted.

It was to make it clear during usage that there is no password involved as someone reading it may interpret as entering an empty password.

Fixed.

Thanks for the feedback! Turns out String.split() will return an array with 1 element when the string is an empty string. A similar problem in UnlockCommandParser has also been fixed.

I agree, I have updated the method's name to improve clarity.

You are right. Actually carrot probably refers to just ^. Will be changing to what you have suggested.

I believe the test name is correct because I'm testing the equals method to be successful when two UnlockCommand with the same currentPassword fields are tested to be equal.

No idea why gradle build fails on Github when it passes on my system.

lgtm

Thanks for the review. Will be merging it

Some of the links can be updated to point to our repo. Currently they are pointing to ab3's repo.

I recommend you clone this https://github.com/swayongshen/tp/tree/add-lock and do some general testing.

I have implemented the new feature of saving the old password. When the user calls the lock command to set a new password, the password will be stored in an encrypted file named keystore in /data/.

After unlocking ClientBook, the next time the user tries to lock ClientBook without a password, the old password will be decrypted from the password file and added to the state of the program and the password file will be encrypted immediately.

You might want to consider removing this portfolio line that is a placeholder.

You might want to consider following the syntax for writing command line arguments here. In this case, the format should look something like week {WEEK_NUMBER | first | next | prev | last}

You could consider changing this to say week 2 shows the second week of the year.

You could also consider using the amended week syntax above here as well.

For these two lines, please include P / E respectively in the indices for the delete command

You might want to review this merge conflict...

Please remove the backup folder from your pull request as well.

There's another merge conflict marker here as well you might want to fix this

This is also another merge conflict marker

You might want to remove this as well

You might want to delete the .backup file as well

Here's the end of the merge conflict marker, you're supposed to tell them which one you want to marge in because Github couldn't do this automatically

You might want to remove this portfolio link since it links to an empty file

You might want to add a comment on why this part is commented out.

Might want to change to EventBook here

EventBook*

Similarly, you might want to consider leaving a comment on top of this commented chunk

You might want to consider removing the comment since you're done implementing it

You might want to consider changing the variable name to BACKLOG_EVENT_STATUS

You might want to consider changing the name of this to IN_PROGRESS_EVENT_STATUS

InProgressCommand**

BacklogCommand**

Event should be edited based on their identifier, not their index.

Define another method getEventByIdentifier(int identifier, List events) to get the event by identifier


Event getEventByIdentifier(int identifier, List<Event> events) throws CommandException {

    List<Event> eventsFilteredByIdentifier = events.stream()

             .filter(event -&gt; event.getIdentifier() == identifier)

             .collect(Collectors.toList());

    if (eventsFilteredByIdentifier.size() > 0) {

         return eventsFilteredByIdentifier.get(0);

    } else {

        throw new CommandException(Messages.MESSAGE_INVALID_EVENT_DISPLAYED_IDENTIFIER);

    }

}

You might want to consider converting the user input to uppercase as well, since currently, if user passes in s/todo, this would get rejected as they would need to do s/TODO, which might not be very good UX.

If we uppercase the user input, then the user can pass in s/tOdO and it will still work.

You might want to consider renaming index to identifier, since we would be checking by identifier, instead of index.

Closed due to cyclic dependency

bullet points starting with uppercase letter

should we remove the parentheses?

"confirm crucial commands with a confirmation message"

2a1

this backtick should be at the end of line

missing triple dash

missing full stop

and Prompts

format should be delete schedule

find

change back to Prompt: by module/day/week

removing the 3 lines starting from this line fixes it

for the tick

double whitespace 👀

task

FindTaskCommand

task

tasks

you might have forgot updateFilteredScheduleList, i'll merge my pr first

might wanna change the usage because we are not deleting by index

the ellipsis at the end looks like it's asking for more parameters but in fact, the TAG parameter is supposed to be spread. i'm more towards explicitly defining [t/TAG] as 0 or more TAG arguments expected

as an argument

i do not see your activity diagram in developer guide at all. did you forget to include it?

will modify it as EntryTagContainsKeywordsPredicate

reopening. missed project description and credits to se-education

closed via #11

refactor UniqueTaskList to NonOverlappingTaskList

obsolete, refer to #80


        Use case ends.


        1a1. HippoCampus shows an error message.

        Use case ends.

Extra space.


            + "for more information refer to https://ay2021s2-cs2103-w16-3.github.io/tp/UserGuide.html\n"

Extra newline.


            + "help [COMMAND]";

Missing exit command.


            + "edit INDEX [-n NAME] [-p PHONE_NUMBER] [-e EMAIL] [-a ADDRESS] [-t TAG]…\u200B [-b BIRTHDAY]\n"

            + "Exiting application: "

            + "exit\n"

This is a little confusing. By deleting the contact when no more tags exists, it suggests that contacts cannot have no tags. But what about a workflow where user wants to delete all tags, then assign new tags from scratch?

Should the delete -t command just deal with tag deletion alone, and not touch contact deletion at all?

I think using the current add, find, delete for contact processing is a good idea, like what you mentioned. That's essentially the existing commit: "deletes all contacts with at least one of colleague or cs2103 tags".

It's the proposed change that I don't quite catch, i.e. "delete tags in contact + delete contact if there is an exact tag match".

PS: On this note, going by your suggestion to use tags exclusively for tag, we can do tag deletion using the same command, e.g. tags -d -t colleague -t cs2103. Sounds like a pretty self-consistent idea.

Cos the happy path is to delete the contact. Then ONLY IF it is tagged with another tag then the contact is not deleted.

What about the case where I want to delete all people with that tag? I can't quite think of a viable workaround for this, except manually searching for and deleting the person, e.g.,


# Removing everyone tagged A requires `delete 1 2` instead of `delete -t A`

1. John (tag: A)

2. Grace (tag: A, B)

3. Bob (tag: B)

But I guess these issues only arose because we didn't define the feature well enough in the feature request.

Missing space.


            + "Example: " + COMMAND_WORD + " -n Bob -t CS2030";

Probably an extension of using string validation, difficult to predict all possible user errors.

Might want to change to } else { style recommended in this module.

Screenshot of what @garyljj is referring to:


     * Parses a {@code String remark} into a {@code Remark}.


     * @throws ParseException if the given {@code Remark} is invalid.

Perhaps best of both worlds: "You can write anything in the remarks, as long as it is not blank".

Perhaps a more natural phrasing?


    public static final String MESSAGE_DELETE_PERSON_SUCCESS = "Deleted the following people: %1$s";

Might have to consider a different phrasing later, once the --append flag for tags is added in as well. Should be fine for now.


            + "OR " + COMMAND_WORD + " " + FLAG_REMOVE + ": Removes specified tag from all people in the filtered list.\n"

The recursive string concatenation is technically quadratic here, though it's probably negligible for small number of Strings. Can consider .map(...).collect(Collectors.joining(", ")); from this SO post.

If implementing, can consider allowing removeTagFromPerson to return a boolean reflecting the tag removal success, just like the boolean isEdited = tags.remove(targetTag); line you used in line 49.

On a related note, since model.addState() should not be called if no tag removal operations were performed, I think doing the check would be a good idea. Ties in with the same idea.

Sure. If got time, I can probably do a quick separate PR for all occurrences.

@zhengruoxin yup, also noted here. I couldn't find a way to solve this invalid date mapping problem, without having to move towards manual date parsing. Was planning to document this quirk in the UserGuide and leave it be, since adding the wrong date is not within normal use anyway.

Sounds good, it should be more efficient that way. Will make the change.

This should be better:

Assuming in typical use scenarios, the number of unique indices specified is not large (pretty unlikely)
Since the application does not require strong performance optimization, readability should be a priority

Will apply suggestion and merge.

Remove references to HashSet with change in algorithm, discussed below.

Non-functional requirements

Quality requirement: HippoCampus should be usable by a novice who has never used a CLI addressbook before.

HippoCampus should be able to hold up to 1000 contacts without noticeable sluggishness in performance for typical usage.

Should store data locally only, in a human editable text file, for privacy reasons.

Technical requirements: HippoCampus should work on any mainstream OS with minimally Java 11 installed.

Notes about project scope: Should only be for a single user and should not require interaction with other users of HippoCampus.

A user with above average typing speed for regular English text (i.e. not code, not system admin commands) should be able to accomplish most of the tasks faster using commands than using the mouse.

The source code should be open source.

Glossary: Mainstream OS: Windows, Linus, Unix, OS-X

Duplicate of #17

Duplicate of #19

Description expanded in #22 #23 #24

LGTM

No guarantees it'll work right out the box, but it's a start.

Confirmed working. Can close once all checks are done.

@nickyfoo commit at d1d7501. All completed.

Adhere to more generic command line conventions

For a more extreme (cherry-picked) examples, see these workflow runs:

MacOS delayed the check completion due to running 4+ mins.

Alternatives to dropping workflows include using skip actions keyword in commit message to skip tests, but not a healthy habit to adopt.

Reference: https://github.blog/changelog/2021-02-08-github-actions-skip-pull-request-and-push-workflows-with-skip-ci/

LGTM

Courtesy of @nickyfoo, cannot reduce automated tests until one iteration is complete, for grading purposes:

https://nus-cs2103-ay2021s2.github.io/website/admin/appendixE-gitHub.html#tp-workflow

After following the given workflow for at least one iteration, optionally, you may adjust the process rigor to suit your team's pace. Here are some examples:

Reduce automated tests: Automated tests have benefits, but they can be a pain to write/maintain. It is OK to get rid of some of the troublesome tests and rely more on manual testing instead. The less automated tests you have, the higher the risk of regressions; but it may be an acceptable trade-off under the circumstances if tests are slowing you down too much. There is no direct penalty for removing tests. Also note our expectation on test code.

Reduce automated checks: You can also reduce the rigor of checkstyle checks to expedite PR processing.

Switch to a lighter workflow: While forking workflow is the safest (and is recommended), it is also rather heavy. You may switch to a simpler workflow if the forking workflow if you wish. Refer the textbook to find more about alternative workflows: feature branches workflow, centralized workflow. Even if you do switch, we still recommend that you use PR reviews, at least for PRs affecting others' features.

Can postpone discussion until the time comes.

Duplicate feature already present as a Tag in AB3.

Duplicate of clear feature already present in AB3.

In particular, adjusting sample data to follow new functionality.

@zhengruoxin perhaps some ideas on additional datetime formats that we can provide: https://github.com/pyuxiang/ip/blob/master/src/main/java/duke/parser/DatetimeParser.java

Probably here, where the dimensions for the result display are specified:

https://github.com/AY2021S2-CS2103-W16-3/tp/blob/1d205c019aa61650b61f437bc8d77a82bfa0b5c2/src/main/resources/view/MainWindow.fxml#L42-L47

Overall window size seems to be specified here:

https://github.com/AY2021S2-CS2103-W16-3/tp/blob/1d205c019aa61650b61f437bc8d77a82bfa0b5c2/src/main/java/seedu/address/commons/core/GuiSettings.java#L11-L14

Experimented a little with dynamic resizing by setting VGROW to ALWAYS, but it scales together with the person list UI pane, which seemed a little weird: there is no longer an intuitive fixed reference by which the application grows.

For more text, I think the more appropriate action is to limit help text to at most 4 lines.

According to the UserGuide, we're also removing the need for -n to specify the name, i.e. add John instead of add -n John, but not yet implemented. We can open a separate issue for this.

Also need to change the help message.

Issue #81 opened to address the -n prefix issue.

@Ellevy mentioned that find and edit also use the same -n prefix, can maintain in add for consistency.

The todo is now to change the syntax in the documentation to include the -n prefix.

Courtesy of @glennljs: Issue only happens when gradle run command is used. If built normally and open the jar, it works fine. Perhaps grade adds the font as a separate resource internally.

If not fixing, can assign tag::wontfix, remove the priority label, and close the issue.

Duplicate of #49

Duplicate of #67

PR erroneously requested, closed.

Note that merging this is a big priority - will introduce a lot of merge conflicts with subsequent PRs.

Doesn't the current algorithm involve iterating through the list to check for tags? A quick is_tag_found boolean flag probably works.

If want to extend to existence of multiple tags, can create a boolean flag array as well.

Duplicate of #86.

Any suggestions to implement this? Thinking of an InputHistory class native to the parser / executor class. Probably also need to register event handlers for up/down arrow keys in the input field box, see this SO post for a possible implementation guide.

@zhengruoxin just a heads up, in case you're busy, I'm currently working on a new PR to incorporate the use of LocalDate for validation. Should be done by today.

On Fri, 12 Mar 2021, 13:06 Zheng Ruoxin, @.***> wrote:

@.**** commented on this pull request.

In src/main/java/seedu/hippocampus/model/person/Birthday.java >https://github.com/AY2021S2-CS2103-W16-3/tp/pull/88#discussion_r592912610> :
   value = EMPTY_BIRTHDAY_STRING;
   isEmpty = true;
}

/**
* Returns true if a given birthday is an empty birthday.
*/
public static boolean isEmptyBirthday(Birthday birthday) {
   return birthday.isEmpty;
}

/**
* Returns true if a given string is a valid email.
*/
public static boolean isValidBirthday(String test) {
   return (test.toCharArray()[4] == '-') && (test.toCharArray()[7] == '-');
That's true. I considered LocalDate but wanted to keep to standardising using isValidBirthday boolean to check for validity. Will try to see if I can use both, else will switch to LocalDate. Thanks!

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub >https://github.com/AY2021S2-CS2103-W16-3/tp/pull/88#discussion_r592912610>, or unsubscribe >https://github.com/notifications/unsubscribe-auth/AG2V5DVZTIYF25ESV2G6A2TTDGOMZANCNFSM4Y34E7SA> .

Doing iterative addition of features, sample data will be updated concurrently. Not a singular feature.

Personally I'd avoid the highly ambiguous "11 Dec 02", partly because it's uncommon, and will succeed without raising errors for international users whose local formats follow "yy/mm/dd" instead of "dd/mm/yy". Perhaps enforcing the "yyyy" format will catch potential errors, or another option would be to allow users to customize the desired date format.

This might be a good reference: https://en.wikipedia.org/wiki/Date_format_by_country

Perhaps the formats we might want to support include the predominant little endian DMY format (exception is ISO format which should always be supported):

"yyyy-mm-dd" ISO-8601
"d.m.yyyy" German, Russian
"d/m/yyyy" French, Spanish
"d-m-yyyy" Hindi

with the associated long forms:

"mmm d yyyy" USA (+ "mmmm d yyyy")
"d mmm yyyy" Singapore (+ "d mmmm yyyy")

Closed, not implementing, since bottleneck is no longer workflow runs, but PR reviewing itself.

Implemented by #96

Is the deletion done across the board, or just for the filtered list?

Got confused a little: Isn't the undo command feature part of your PR, and not yet integrated into the master branch? Can raise this issue in the PR first, then open an issue from there if not implemented.

What's the syntax to test this?

Courtesy of @nickyfoo: Just "undo". It will undo the last change to the AB (by add, delete, edit or clear).

INFO: Added state to stateHistory. Current number of states is: 2. Currently on state: 1 is the logged message. Seems like we can have a feature to easily redo as well.

Merge master branch by updating newly introduced Remark feature to include long-form "--remark" prefix in CliSyntax.

As per previous discussion, this GUI design will be phased out in favor of Event cards (see commit 02452dd).

Part of the reason includes: (1) lack of horizontal real estate, (2) not a critical feature (selection of tags will be supported by commands in the upcoming PR #116).

Depends on which we choose to implement. The alternative, with perhaps the flag --exact, is also viable.

Even if decide to keep it as exact by default, i think would be good to minimally make it non case-sensitive.

Yes, exact is already case-insensitive.

Changed default behavior to partial matching as requested by @garyljj.

Should filtering by "people who do not have a birthday specified" be implemented as well? From the perspective of the welfare IC, they might be interested to check for people whose birthdays are not yet populated, so that they know to approach them for this additional detail.

LGTM

Are we going to throw error if the user types in wrong specifications? like eg. list -partial or list partial or list Bernice. Currently if i'm not wrong it just ignores and lists all people in the addressbook

Thanks for spotting the bugs, you're absolutely right! At first was thinking of making space for the discussed list 1 3 syntax, but that's not the scope of this PR. Have added changes to check for the required empty preamble.

Some notes:

list -partial and the other variants above are now invalid.
list -partial -n john will not be valid since the preamble -partial exists.
list -n john -partial will be valid since the search term for name is now john -partial.
- This issue cannot be avoided since this is fundamentally how the parser is designed, to only parse specified prefixes.
- One way is to enforce regex validation, but we probably can't predict how people name themselves / their kids 😂

Just need to update HelpCommand.java

then LGTM

Great catch, thanks! Let's defer this till once the list functionality is complete, since there are more pending changes to the command.

Reopening this, issue still persists from https://github.com/AY2021S2-CS2103-W16-3/tp/pull/107.

Currently only allows deletion of 1 tag at a go (only the last specified tag will be deleted).

eg. edit --remove -t friends -t colleagues will remove colleagues tags from all listed persons. Should we allow deletion of multiple tags at a go?

Technically the proposal does specify multiple tag removal, but the syntax didn't reflect it :p

If implementing, can use argMultimap.getValues in line 92 of EditCommandParser.java.
If not implementing, might be better if we change the success message to include which tag was removed.

Did we conclude whether we wanted to do this, the other time we had a discussion?

Do we want to allow sorting based on multiple conditions? eg. I have 2 people of the same birthday and I want to sort based on ascending name then descending birthday. If we do not want to allow it then should we throw an error when a person inputs more than 1 -s?

Probably not, since it increases the complexity of the command + the parser doesn't distinguish the absolute positions of the prefixes, so matching two prefixes will be a pain.

Throwing error is one way, and the ~~easy way out~~ other way would be to simply document this ~~behavior~~ feature (i.e. only the last sort prefix given is used), just like for the add command. Which do you prefer?

I think we agreed that memory not really a concern atm since undo history doesnt carry over across sessions.

Floating an idea, what about (1) setting undo history to a cap higher than the average number of commands used in a single session, and (2) letting undo history carry over sessions? It's a little like vim's swap files.

Validation of the swap history can be achieved by storing the latest address book state in a lightweight hashed format, so it avoids users abusing the swap file to trigger invalid undos.

Do we want to allow sorting based on multiple conditions? eg. I have 2 people of the same birthday and I want to sort based on ascending name then descending birthday. If we do not want to allow it then should we throw an error when a person inputs more than 1 -s?

On that note, forgot to mention that if this is desired, there is a workaround for it, e.g. to sort by ascending birthday, and descending name if same birthday, can call these two commands in succession:

list [...] [-s name] -o descending
list [...] -s birthday [-o ascending]

I agree, though I also think the UserGuide is a good starting point for the users. Perhaps we can consider link drop in the result pane when help command is called.

On the same note, I think a nice feature would be to remove the menubar feature. Takes up much needed real estate only 😜

Gave a cursory glance (no computer to test), looks okay. Note the date comparisons are a little different between EventDate and Birthday:

EventDate should be sorted by date including the year, whereas Birthday is by month and day only. The compareTo method in Date should implement the absolute ordering, and Birthday to override this behaviour using getMonthDayString.
When merging #126, the retrieval of the string date for comparison should return "" for empty Dates. This is noted in the PR description.

--any is not the opposite of --exact functionality

Tested using #176 head. Very straightforward LGTM.

Dark theme:

Pastel theme:

As per internal discussion, will not be implementing due to lack of potential usage.

Great suggestions! All proposed changes have been integrated.

It seems like this exception is relating to session, but i'm not sure if it should be named as IllegalArgumentException, since there's the default java exception IllegalArgumentException. Might be confusing in future. Perhaps we can further discuss this? @JonahhGohh

🎉

minor bug: find_session KEYWORD instead of find_student KEYWORD

Perhaps we could consider the test case where it starts with 6/8/9, but has >8 digits in total, instead of this current one (or we could create a new one). Not sure how important it will be, but feels good to have!

I believe should be k/120 instead of l/120, and t/18:00 instead of t/1800!

Perhaps we could include an assertion for invalid days in month, for example 31 Apr is invalid since April only has 30 days, something similar to leap year check! (does the exception catch this actually?)

This should evaluate to delete_session John Doe 1 but should be delete_session n/John Doe i/1

HAHA this one abit ocd but perhaps make all the points not end with a dot for uniformity, otherwise lgtm!

think you forgot to remove the comment right here

Are these commented codes rough work? Might be better to remove.

that's right!

I'm not sure if the bracket filtered feels abit out of place. Maybe something like Listing students' emails based on current list might be better? Hahaha, maybe someone else can phrase it better!

Nice catch on the missing delete_student!

I think the header can be "Fees"?

Then under action "Student fee by month", what do you think?

I'm assuming the method is named parseStudyLevel to be uniform with the rest. However, for study level, it isn't exactly 'parsing', since it's String -> String. As compared to say email, where it's String -> Email.

Perhaps without changing method name, we could change the description to "Sanitize/Clean studyLevel input"? or something along that line. Don't think it's a big issue tbh, but what do y'all think?

Same goes for relationship as well!

I think this might violate the 'explain WHAT and WHY, not HOW' @ textbook.

Same comment as above (regarding WHAT and WHY not HOW).

Honestly, i don't have a clear idea of how to phrase as well 🤣

One way is to remove comments, since it's pretty self-explanatory (as per textbook), maybe someone else has a better suggestion? @AY2021S2-CS2103T-T11-1/developers

Minor change: Student instead of student

Do you want to mention about how the filteredStudentList is updated as well?

I think 'UI shows updated student list' comes before 'Logic saves the address book to the storage', according to the code:


commandResult = command.execute(model);

  try {

      storage.saveAddressBook(model.getAddressBook());

  } catch (IOException ioe) {

      throw new CommandException(FILE_OPS_ERROR_MESSAGE + ioe, ioe);

  }

You can double check!

How about a test case to consider when all/some parameters are null?

new Tuition(null, null, null, null)

Perhaps {@code Session}?

The chunk (from line 114 to 138) is commented out using HTML >!-- --> tags! So should be irrelevant for now.

Yup, I agree. something we can keep in consideration beyond v1.2.

Oh and as for the comment out, the chunk (from line 114 to 138) is commented out using HTML >!-- --> tags! So should be irrelevant for now.

Commenting out in the middle of list items creates a bit of a weird gap in between. I'll take your suggestion and comment it out at the bottom, so we can restore it easier next time!

I think can imagine the listener as something that's running in the "background" and always monitoring the ObservableList>Student>, so whenever there's any changes to the list of students, the code within the block will be invoked. As for the change.next(), if there are multiple changes made, they're represented as a 'separate change' (list of changes), so change.next() loops through it (might not be 100% correct in this part).

The ListView>Session> is actually bound to the ObservableList>Session> sessionList, so whenever there's a change in the sessionList, which is shown in this block of code you commented, the ListView will update as well.

(might not be 100% correct)

Amended in next commit!

I think that's how it's supposed to be, just like in JsonAdaptedStudent

Will be done in 1.3!

No sir, it should be students.deleteSession(student, sessionIndex);

Amended in next commit!

It's activity and sequence diagram! hahaha

Oh yes, i also found the indentation weird as well. Will make the change, thanks!!

looks good to me as well, nice catch, will proceed to merge.

Made changes mentioned by @yungweezy @enhao25, merged! 🎉

@samleewy I think the comments change and parse method name change, I'll create a new issue so that we can discuss. For now, maybe you can approve this first to fix the trim date bug?

sure!

LGTM!

What do you having this condition as "valid command input format" instead? I feel like "command exist" is not very clear 😅

@JonahhGohh ok will change, thanks!

Need to update the documentation here

Update documentation

Add a vertical spacing here? - the auto-formatter should take care of this

Same issue with ManufactureDate, CompletedDate and OrderDate

If Order has a cheeseType property, then I would assume that one order should only have one type of cheese. Then, I think there should be some checks to ensure this?

An alternative would be to let an Order have multiple cheese types. If so, then we will have to remove the cheeseType property.

Update documentation

Add documentation

Is this class used anywhere yet?

I believe these should be placed under testutil instead (not logic/commands/CommandTestUtil.

Also, it will be great if we tested invalid dates and cheese types as well.

Standardize all comments to start with capital letter?

Should we rename FindCommandParser to FindCustomerCommandParser? In order to be consistent

Currently some funny things are happening to the customer list after we make the findcustomer command with an empty field following a valid prefix, for example, after inputting "findcustomer n/".

Perhaps we can consider using ParseException to handle the above case as well?

I think we should have two cheeseIds in this completed order? Since its quantity is 2 and we should not have been able to complete it with only one cheese.

Changing the quantity to 1 will work too

I think we need to account for the case where the input cheeseId no longer exist.

For example, suppose we have order with OrderId 3 that has been fulfilled with cheese with CheeseId 3.

The user first deletes the cheese with CheeseId 3.
The user proceeds deletes the order with OrderId 3, which brings us to the code block above.
The above for-loop is unable to find the cheese with ID 3, since it has been deleted previously.
this.targetIndex is set to Index.fromZeroBased(1), the default value.
The wrong cheese is deleted.

I think we should "unfilter" the order list here? That is, calling model.updateFilteredOrderList(PREDICATE_SHOW_ALL_ORDERS). This is so that we can iterate through the complete order list when finding orders to cascade-delete.

This is so that when the findorders feature is implemented in the future (which will cause the order list to potentially be incomplete), this command can still work fine.

Think we can "unfilter" the cheese list here before iterating through it! Rationale below (in another comment).

We might not be able to use Index.fromZerobased(0) as the base case representing that a cheese is not found.

In the below for loop, if the matching cheese is in index 0, then we will need to use Index.fromZeroBased(0) as the targetIndex.

So, need to either change the base case or the loop below.

The two ways when handling invalid data file:

Data file not in correct format -> warning -> start with an empty address book.
Data file not in correct format -> IllegalArgumentException -> app does not start.

Should we standardize this to one method??

I think we can add in a case to handle when the user keys in an expiry date but not a maturity date when adding a cheese.

Anyway, since we are at this topic: I was thinking, we may want to consider dropping certain attributes from the models. For example only, since we have only 1 week left to add features, I think we may not have the time to implement features that deal with the cheeses' manufacture date, maturity date, etc.

We can always leave the clean up to v1.4, so it's just something to think about now? Should discuss more during Saturday's meeting.

I think it's okay if the cheese ID does not currently exist?

Consider the following scenario:

User adds a new cheese (CheeseID = 1, CheeseType = Gouda, Quantity = 1)
User adds a new order (OrderID = 1, CheeseType = Gouda, Quantity = 1)
User completes Order 1 with Cheese 1.
User deletes Cheese 1 (we currently allow this to happen without changing the order to which the cheese was assigned).
User close the app.
User reopens the app and runs into an IllegalArgumentException because CheeseID 1 no longer exist.

We can also check to ensure that an incomplete order's cheeseIds is empty.

If not, the app can start without errors or warnings even when there's an order in the data file like this:

I think disallowing cheese deletion after it has been assigned sounds good - it will be just like IRL where you can't throw away a cheese after assigning it to an order, for example.

This one you and Li Quan can decide? or you want to wait until Saturday to discuss also can.

And then, actually a cheese manufacture date don't necessarily have to be after an order's order date right? Are you referring to the order's completed date instead? If so then sounds good!

Okay i just removed the bolding entirely, thought it was unnecessary (dk why I typed it this way, must have missed it).

done!

ahh I get what you mean. Made the change

It's quite hard to make changes to the UI and test them in this current branch, the reason being we cannot yet create observable lists of orders or cheeses.

Is it okay to merge this branch first? Then I can create another branch, rebase on the new storage branch and work on the UI right away.

Ignore last ^. I'll rebase on the new storage management branch and work from there.

Noted, will address this before tmr (monday) morning

Noted, will address this before tmr (monday) morning. Will probably be adding a class to Model to reflect the screens to show.

mb, fixed liao

mb, forgot to change the methods' and variables' names after copy-pasting - fixed

loooks good to me!

Trailing white space

!aliasesOptional.isPresent() -> aliasesOptional.isEmpty()

Might want to follow the addressbook initialization


initialAddressBookData = addressBookOptional.orElseGet(SampleDataUtil::getSampleAddressBook);


initialAliasesData = aliasesOptional.orElseGet(SampleDataUtil::getSampleAliases);

Missing javadoc for aliases param

Should we document this unchecked exception?

!jsonAliases.isPresent() -> jsonAliases.isEmpty()

Return value of parseTagsExceptLast not being used and Javadoc missing. May want to consider returning void?

References found in:

AddCommandParser#L100
EditCommandParser#L129

Will need to add PREFIX_REMARKS from PR #77

Duplicate chunk found in AddCommandParser#L84-105, may want to refactor it out?

Can be replaced with positions.sort(Comparator.comparingInt(PrefixPosition::getStartPosition));

Can make aliases final

May want to make the happy path prominent: https://nus-cs2103-ay2021s2.github.io/website/se-book-adapted/chapters/codeQuality.html#make-the-happy-path-prominent


        if (helpWindow.isShowing()) {

            helpWindow.focus();

        } else {

            helpWindow.show();

        }

Sure!

Got it, maybe later on we can implement a sample alias in

Agree, https://nus-cs2103-ay2021s2.github.io/website/se-book-adapted/chapters/codeQuality.html#avoid-magic-numbers

Should be LogsCenter.getLogger(getClass());

input is not used

Should this test be ensuring that all items in the list starts with 'a' rather than just index 0?

Might want to test to make sure that empty spaces returns an empty list:

[null, ""]: non empty list

[" ", " ", " "]: white spaces return empty list (is this the intended behaviour?)

["e", "ex", ...]: non empty list

I've also realised that the method getAutocompleteCommands in Logic.java isn't documented.

Slight typo here. Other than that, LGTM.

Why is there empty line here?

I accidentally added this duplicate @ ce88d0f01172233e0097c0b35f3d39f32c7a4180

Ah yes, it should be true if it will be shown and false if it should be hidden.

What do you think of rephrasing it like this:


/**

 * Returns predicate that determines field control visibility.

 * @return predicate that returns true if prefix linked control should be shown

 */

Added more test cases @ 1e159f4e080ebf9107aa3e93e24bb7c8b32d70a2

This line should be checking for same object


    @Test

    public void test_predicateEqualsCheck_allEquals() {

         ...

         assertEquals(addressArgs, addressArgs);

         ...

https://github.com/AY2021S2-CS2103T-T12-3/tp/pull/70/commits/1e159f4e080ebf9107aa3e93e24bb7c8b32d70a2#diff-0d3609fd2c6dc142dc264b379879b18ceb04ec5efcd9e1ba87f410b4070f4b4aR47

LGTM, but perhaps might want to rename PR to something more appropriate. See #52

Git Commit Name Convention

We will stick to conventional commit style, but we will capitalize the first letter after the colon.

Recommended examples:

docs: Update readme.md
docs: Update README.md

The general consensus is that if it conveys what this commit does, it's fine.

Git Branch Naming

Naming of the branch doesn't really matter as long as it makes sense.

https://se-education.org/guides/conventions/git.html

If a branch is related to an issue, it is recommended to use the format {issue number}-some-keywords-from-the-issue

Recommended examples:

docs/69-update-readme
feature/22-add-storage-backup

Pull Request Naming Convention

No specific PR naming convention, as long as it conveys information on what that PR does.

Can use conventional commit style or write your own short description.

Seems like the changes in PR #65 are working. I have closed the v1.1 milestone since there's no more issues related to it.

In ModelManager, should line 107

updateFilteredPersonList(PREDICATE_SHOW_ALL_PERSONS); of

public void addPerson(Person person) method

be updated/removed to ensure the filter is persistent even after adding a person?

Other than this, everything else LGTM for Logic component.

This line should not affect the filter after adding a new person

I realised that the help text for add command does not include spaces between the flag and the value. I'm guessing the edit command will have this issue too

Sort order for list changed after doing a find command. Should list command show the original order?

@oeiyiping will you be updating the diagrams for your changes to Remarks?

https://github.com/AY2021S2-CS2103T-T12-3/tp/pull/108

Seems like the config still doesn't work well. Might want to disable codecov/patch

JAR released: https://github.com/AY2021S2-CS2103T-T12-3/tp/releases/tag/v1.2.1

I think maybe we can use student instead of stu to improve readability

Possibly change the use of 'acc' and 'el' here too

The change.next() method controls the position of an iterator that goes through the discrete changes in the Student List. Whenever there is a change the listener catches these changes and triggers the contents of the onChanged function. The while loop will terminate after all tracked changes are iterated. Here's the API

Yeah, I believe the "calling" to update the ListView is abstracted away in the implementation of ObservableLists in JavaFx under the Observable collections.

I think the reason why we're using ObservableLists is properly summarised in this article.

A ListView bounded by List will not update upon changes, but a ListView bounded by ObservableList will.

Correct me if i'm wrong, is this a particular coding standard I do not know of?

Might not need to check for same sessions?

{@code name}

I believe this.value would be better? 🤡

I believe this.fee would be better? 🐔

Lazy efficient way, I like! 👍🏼

I believe this.value would be better? 🐷

whitespace missing after @JsonProperty

Are we leaving these commented out?

This removeSession method seems to be very destructive. Could this probably be student.getListOfSessions().remove(sessionIndex)? 💥 💣

Oh shizzle mah nizzle. Roger doger!

Hahaha, wait I placed them in brackets because I'm not sure which term we should go with 😄

Maybe a typo here? 😅

Not sure if capitalization of session here would make a difference.

Not sure if capitalization of session here would make a difference since it is referring to the class Session in the javadocs.

Might want to add the command deleteSession in UniqueStudentList.

I am not entirely sure if the inclusion of the seedu.address and model boxes adds much value here. Maybe the rest can look into this too!

Hmm is this change here intended? 🤔

For the architecture sequence diagram, perhaps we can change it to a more generic example that @enhao25 sent on the group so that we don't need to update if we were to change commands formatting, and to use this diagram to show the interactions that occur for all commands. ☕

Agreed, I will make the necessary changes.

Agreed, I will make changes on the next commit

Will need to write test cases for Students!

I will check again if there are conflicts with test cases. But the reason for the change was to fix a formatting for the study level field because currently some of the entries differ, eg. "P5", "Sec 2", "JC 1. I understand the consideration for possible less trivial cases like "Uni CS3230", but I believe we should standardise the format, at least for pre-generated data.

Did you mean line 80?

Possibly for use in the future to differentiate session objects, since we do not assign unique ids to sessions yet. If this is not needed I can remove it!

Yes, I wanted to check if the same student with different list would assert to be equals, however I had forgotten to change BOB back to Alice nice spot!

Yes you are right, I will resolve in the next commit!

ohmilorde thanku

Placeholder for next iteration where we add student names into session, will remove for now.

I would think so, so that we know the two panels are in sync and the first session will always be from the first student in the list. What do you think?

Good point 😊 Will make changes!

Yes correct, this is because of the OneBasedIndex that is returned in getSessionIndex(). I will add in a comment to make this clearer!

Answered in previous comment!

Thanks for spotting, will update.

Hahaha, yes I am aware, just thought that I might need to change it next time so might as well copy first. Will remove later on if it is not needed!

Were these from AB3? Okay I'll remove the extra forward slashes.

Thanks for spotting! 🙏🏻 arigathanks

Gotcha

Duplicate commit

A sketch of the UML with a Tuition class that contains both Student and Session class.

Realised it doesn't add much value to @JonahhGohh's UML but here's a glimpse if anyone's interested.

I also noticed all the commands that @enhao25 changed to name it as ____StudentCommand got reverted. Is it intentional?

It is intentional if we were to follow the current implementation of Tuition class, where all student parameters are explicitly declare in the Tuition constructor.

So there will only be 1 TuitionCommand which is the AddTuitionCommand right? The rest like list, delete, find, etc are still StudentCommands?

This is a mistake, good spot, should be AddStudentCommand.

Resolved in #43

Resolved in #34

Change of implementation to hold list of session as attribute of Student class.

Dear me 😭

Closed due to change of implementation.

Resolved in #53

Pulled to #67.

Pulled to #73

Some comments on testing.

Can you add a parseSessionCommand_list() test case in AddressBookParserTest? #76

Okay! I'll add them in.

For quick reference, here's an image of the current UI:

LGTM. Not sure if we still need the d in 'd:ListStudentCommand' in your UML sequence diagram. I think that d was used in DeleteCommand and it might be confusing here. I think the best representation is to remove the 'd's all together.

Okay will make changes on the next commit

Yeap LGTM. However, there is something that I noticed when checking out the PR and testing it which is outside of the scope of this issue. The duration class allows a duration of 0. Do you think it would be better if we only allow value > 0.

Good point, I'll add a data validation for this!

Maybe you can consider correcting imports with *? Could be why your code failed the style check.

you could actually just add your new property details to TypicalProperties file under testutil rather than editing the default values of property builder

Should it be property book instead of address book here?

Remember to update the javadocs here too

haha think it shouldn't matter but for future reference I think u should just add it to the TypicalProperties file ba 👍

a suggestion bc I don't think its good to initialise the variables to null:

Name name;

Contact contact; etc

consider use of split function for more concise code?

oh I see, because the toString may not contain the variable. ok nvrm then haha

okk I see that u updated the function to use length function 👍 I just found the '13' and other indexes to be random

Should this be combined into one line?

Should this be combined into one line? 120 characters per line does not apply to DG

Should this be combined into one line?

Should PersonListPanel be updated to PropertyListPanel and AppointmentListPanel?

should "the person whose name contains the keywords" from the original address book be removed?

don't think your eclipse setting files should be included

I see, ok

are your diagrams to be inserted later?

these too

should this be 'undo'?

are these to be added later?

fixed!

fixed as well

Fixed, thanks!

fixed

done

Fixed as well:)

Nope, if you guys are agreeable with the new LightTheme Ui, I will go ahead and delete DarkTheme.css

I would say it's not impossible but the purpose of adopting a light theme was to allow the greying out of expired property and appointments to be somewhat observable yet inconspicuous. The grey effect in the already Black/Grey DarkTheme would probably not achieve the same effect. However, if you guys are keen on a dark theme, I can consider a DarkTheme design in a subsequent update- maybe keep the DarkTheme.css file for now

Code style issue at ../src/test/java/seedu/address/logic/commands/RemarkCommandTest.java:40: no newline at EOF.

hey, I've fixed the bug that doesn't display postal code. However, about price, have to consult junwei on this because he is adding the price of the property as client info actually, not as part of property attribute itself

not implemented, to be considered after v1.4

is the space supposed to be there?

should there be two seperate commands for property and appointment?

shouldn't it sort the filtered list?

should it be filterby or sortby? same for the rest below

yeah, sure

waaaah thanks for helping me update mine!!

nice!

is there supposed to be something else after the and?

would it be better to initialize it in the constructor?

i think it could be permanent, but idk

Thanks for doing this!

thanks!

why is the example usage removed?

that makes sense

i like the colour you chose

izzit possible to have something to toggle between light and dark themes?

i see

undone? idk my english is quite jialat

i think there is a typo, i think us should be is

asking price might have decimals too, would using the regex defined in asking price help?

may i know whats changed here?

i was thinking like the recognized fields will still be updated and shown to be updated in the list, should i phrase it differently?

personally, i think i would prefer 1 if i were a user but would also want to be alerted that there were erroneous fields however, i think it would be best to follow the add command. Like if add aborts the whole command, i think edit should abort the whole command too just to be consistent

Ahhh, pesky IDE. Thanks!

Thanks for catching it! should all be fixed now

Thats true, would changing the default values of property builder be alright though, then I wouldn't have to edit again

Thanks for catching this! should be fixed!

You have a good eye! should be fixed!

thanks! fixed!

shld be fixed!

noted, though i should warn you, i would probably be doing the same thing for appointment to keep it consistent XD

IDE complains that the variable might not be initialized if i dont put leh

that would be one way of doing it, i just find this way easier

I was thinking of using this to validate that there is only 1 new/proceed/cancel

but have not written the code out fully yet

yeah, cos it has to implement the status interface, and completion is like the end point. Is there a better way to resolve this issue?

nice catch! i used scenebuilder, it must have changed things automatically

same for this

and this

thanks!

moved it! thanks!

i think you are right, made the prefixes consistent!

removed it!

removed this too! thanks!

added more words, is it better now?

updated!

updated too! thanks for catching it!

added the code to ensure that there is only 1 new/proceed/cancel

oh yeah, nice catch!

added a check to raise an exception if proceed is used on a property with a Completion status

moved it!

same as above

oh yeah! thats a good idea! thanks!

Revamped the prefixes!

yeah, these too

Nice catch! thanks will change it

yepp!

yeah, yet to learn plantUML will add next week

should I close this pull request?

LGTM, thanks for the tremendous effort

i think the refactoring is pretty good, can make things easier

I'm a little confused about why current_password is wrapped in square brackets. Does this mean that if a password has not been set, then unlock command does not need a password?

For the return statement of this method, the comments helped me to understand the code easily which is already great, but I was thinking whether the expression would be a little less complicated if you separate the boolean statement into multiple named boolean variables.

For example


    @Override

    public boolean equals(Object other) {

        isSameObject = other == this;

        if (isSameObject) {

            return true;

        } else {

            isLockCommandObj = other instanceof LockCommand; // instanceof handles nulls

            isCurrentPasswordEqual = currentPassword.equals(((LockCommand) other).currentPassword);

            isNewPasswordEqual = newPassword.equals(((LockCommand) other).newPassword);



            return isSameObject || (isLockCommandObj && isCurrentPasswordEqual && isNewPasswordEqual);

        }

    }

If we remove the password here, does that mean that every time the user unlocks the ClientBook, the next time he locks he needs to set a password again?

If that's the case, then I'm thinking if it is possible that we don't remove password on unlock and store the password, so the user only needs to specify a password for the lock command when he/she first adds or changes his/her password. Then subsequently, if he/she locks and unlocks it will just use the same pasword.

edit: after thinking about this more, I realised that its not so easy to store the password securely within the app (I was thinking in UserPrefs? but not sure if it is secure or even possible), and perhaps this one time lock and unlock is enough. What do you think?

This method is fine, but are the 2 try-catch blocks are mainly to differentiate between the 2 types of exceptions? I was thinking whether you can condense it into one try-catch block, and in the catch block, you differentiate the 2 cases using the ZipException that was caught.

Actually I was looking at the docs, and found that they didn't really specify what ZipException each method threw, but on their ZipException class, there is a getType() method, which returns a ZipException.Type. (https://javadoc.io/static/net.lingala.zip4j/zip4j/2.7.0/net/lingala/zip4j/exception/ZipException.Type.html)

Perhaps you could check if it is possible to differentiate the cases by checking the exception type? If not then your method is completely fine as it is

Just a little bit of unnecessary white space 😃

Is there some reason you chose to overload the attemptUnzip() method with this? It seems to me that you could directly call attemptUnzip("") where you want to call this method.

I see, that makes sense. Perhaps it could be clearer if you leave a comment or rename the method to indicate that it is for cases where no password was involved?

I see, since the API doesn't specify then it's difficult to catch the different cases ourselves. In that case then two try-catch blocks is a nice way to circumvent that👍🏼

I agree that the effort to reward ratio is pretty low since it's not a particularly core feature of our application. In that case, let's keep this extension in mind, and if we have time after other core features then we can possibly come back to it in the future.

I see, then let's follow the format 😁

For the name of this method, I was wondering if the name may be slightly inconsistent with the other get methods of this class, since other get methods in this class returns the object, while this method returns String. A suggestion would be to name it something like nameAndPoliciesToString()

There seems to be a lot of white space here. Is there a reason you chose to have a new line after each of the statements?

Do you think that naming this method formatOuterBox() is more specific? since there are other VBox instances in other classes or if we extend the UI

I see, I think I didn't notice the line breaks were separating each of the variables/objects. now it makes sense!

I suggest changing this to ATTRIBUTE, since we used attribute to specify in other parts of the document

Similar to the previous comment, I suggest using ATTRIBUTE instead of property

This line is a bit confusing, may be instead of "more than 1", it is more suitable to be "at least 1"? Alternatively, we can also say "index must be within the numbers listed on the clientbook"?

A little nitpicking, but instead of leaving it as the default case, it might be better to specify each specific case, like

The CURRENT_PASSWORD field can be omitted if ClientBook is not yet locked.
When CURRENT_PASSWORD and NEW_PASSWORD are both specified, ClientBook verifies the current password before locking ClientBook with the new password. >- specifying this case instead of leaving it as default case
When CURRENT_PASSWORD and NEW_PASSWORD fields are both omitted, ClientBook will attempt to lock itself using the last used password that is safely stored on your device.

Awesome addition of the column!

By the way, I tried to update the data directly from the json file, but not sure if it is because of the zip, but it doesn't work. After I change the json file, I start up the app and it goes back to the previous state. Maybe you guys can test it also, and if it doesn't work, we just remove the "Advanced users are welcome to update data directly by editing that data file." part

Made the changes to differentiate command words in this section in the latest commit

Added tags back in

Added the quick start into the user guide, but release is not yet ready.

Great suggestion, done!

Great suggestion, made the change.

Done!

Good point, done!

Just did some testing, feature works perfectly! The only small issue is when you input unlock without any parameters, the feedback message to user is


Invalid command format! 

lock: Locks ClientBook with a password.

Parameters: CURRENT_PASSWORD(if any) NEW_PASSWORD

Example: lock 12345

And also I think maybe it would make it clearer for the user if we add in the user guide a sentence to specify that when lock is used without parameters, then it will be locked with the current stored password.

After these 2 then its LGTM for me!

Slight change in phrasing:


    * The user should take no longer than 1 hour to learn the different functionalities of TutorBuddy from the user guide.

Maybe can change it to like that to be more specific?


    * A user with above-average typing speed for regular English text (i.e. not code, not system admin commands) should be able to use most of the functionality in TutorBuddy faster using commands than using the mouse.

We do not actually need this file. It is from the tutorial for tP from week 6. I don't even know why this is here. But I guess the change is harmless.

Actually, I was thinking more on the lines of public Tuition(Student student).

I think passing all parameters of student will make the abstraction level of Tuition class 1 level too low.

How about

'Student student = new Student(name, phone, email, address, studyLevel, guardianPhone, relationship);

Tuition tuition = new Tuition(student);'

Should we name this DeleteStudentTuitionCommand for more consistency?

Similar to the AddStudentTuitionCommand, we are operating on a tuition object here and not a student object.

Instead of returning a new Tuition object when we call edit_student, what about returning a student and replacing the student in the Tuition object?

Because the command name is EditStudentCommand which does not really make sense to return a new Tuition object.

I was thinking of tuition.getStudent().getName() but I guess this works well. This is just a design consideration but you can check out "Law of Demeter".

TuitionTest replaces StudentTest. What tests should we have here now?

What is this hashCode() function for?

I think the javadoc comment here should change?

Woops, I was using my iPad to review just now, and I might not have seen the code accurately. No issues!

Oh ok, let's leave it then!

Do you mind explaining the listener part and what the change.next() does? I a bit catch no ball 😅 .

Also, which is the part that calls the view to update the session list every time a session is added?

Good suggestion! The isValidSessionDate() should assertFalse correctly to this test case.

Since we have the isValidSessionDate() check now, we do not need this try and catch block anymore since it is a guarantee that we will receive a valid dateValue and timeValue in our SessionDate constructor.

There are a few changes to other files regarding this issue as well. I will do the changes once this PR is merged.

Is this method needed?

Can we use the showStudentAtIndex method for our session command test cases or do we need to find the exact session in the session list?

Ok! 😄

Should this be AddressBookParser instead?

Hmm, since you mentioned that the AddStudentCommandParser creates both a Student and a AddStudentCommand, would it be better to also mention that the AddStudentCommand executes with the Student object?

Thanks for the catch. I think I might have accidentally refactored all names without noticing. Please see the updated commit.

Ok, I will add 23:002 and 23:592. So to double confirm, 23:002 and 23:592 should throw a SessionException, right?

Yup, thanks for spotting that, it should be k.

@nowknowing it's the prefix for duration (can be seen in the CliSyntax class in the Parser package).

I chose "/k" because "/l" was already used for STUDY_LEVEL in add_student.

If y'all have a more intuitive prefix letter, do suggest!

I use the sessionDate.equals() here instead of session.equals() because I think you created an equals() for Session class already right? @samleewy

I guess i could improve it by bringing the targetSessioDate in the outer loop, since there isn't a need to declare the targetSessionDate in every iteration.

Ok, thanks for the suggestion. I will add some comments to the test cases to make it clearer. 👍

HAHA omg 🥴

Thanks for spotting, will make a change to this.

What do you think of making it like this?

Returns true if session {@code Session} exists in any of the students in the unique student list

I think this takes away the ambiguity of referring to either the session variable or the Session class.

Good point! Can you give an example of how you would phrase it? I'm not sure how best to change it.

Yup! I agree with what you said, idm changing to what you suggested instead. Personally, I think code clarity is more important than uniformity in this case.

Thanks, good catch!

Done!

Removed. I was actually intending to replace the github logo, but the github logo makes more sense in this case.

Add test cases for time 23:002 and 23:592 and both throws a SessionException as expected.

I also noticed all the commands that @enhao25 changed to name it as ____StudentCommand got reverted. Is it intentional?

So there will only be 1 TuitionCommand which is the AddTuitionCommand right? The rest like list, delete, find, etc are still StudentCommands?

I will add test cases for AddSessionCommandTest and AddSessionCommandIntegrationTest later today #61.

Should not cause any issues.

closes #49

Yeap LGTM. Just 1 small comments on adding comments to the equals method. Previously in the group, I think shion also mention that changing the session to store student instead of having student store session could be a possible idea. If we somehow decide to do that, not sure if some of the test case needs to be changed. However, at this point, it might not be time efficient to change, might need to discuss this future when we meet tmr, to see if there is a point in doing that. Else, looks good

I think regarding this, wouldn't it be better to just have these test cases now and modify them later if we were to introduce the functionality?

@samleewy I think the comments change and parse method name change, I'll create a new issue so that we can discuss. For now, maybe you can approve this first to fix the trim date bug?

I think having at "Fees" is fine.

could change contacts to household items

I also think it should be changed back to VALID_NAME_AMY for now just for consistency.

Since this test is a returnsFalse, maybe you shouldn't include an assertTrue inside. Perhaps you could write that in a separate method?

Maybe warning can be in all caps?

U could try using assert throw like in the test right below this. U could also specify what the Exception you are throwing actually is.

Could add some tests since you removed these 😃

I think u can use the above two lines to print the number of items listed. One has the s in items while the other doesn't to differentiate between single and multiple items being returned.

maybe you could say within the next 7 days instead of in 7 days.

similar to the previous one, maybe you could say expiring within the next 2 weeks

u could try using equalsIgnoreCase() like I did for sortCommand

I feel it should be StoreMando since we referring to our app.

The table still displays fine right??

Should these be like UniqueItemList instead of UniquePersonList? Maybe can change this in the future.

I think we can just delete this file also right? If not, can just remove the Level 3

What does this look like?

I think can change this back.

Can just delete the test. Don't need to comment it out

"User wants to search" or "User searches" maybe? Instead of saying expiring soon, can say expiring within the next 7 days or next 2 weeks?

Should a test be added for this?

I felt like the VALIDATION_REGEX does not help to check if the date given by the user is a valid date. So, I used LocalDate instead to check if the date is valid, which can be seen in the subsequent lines. Hope this clears things.

I will add it back. expiryDate will not be null anymore.

I chose to do it this way as I cannot guarantee Long.parseLong(test) will not throw a NumberFormatException. So, I made sure test is made of digits before I check if it is a non-zero value.

Ok. Changed as stated.

alright!

ohh ok i get it. I'll change it. Thanks!

Oh yea I'll remove it! Thanks!

alright will change it!

oh yea my bad

sure will do!

ok will change it

if I'm not wrong, it only takes in days and weeks as TIME_UNIT. I just went to check the reminderCommandParser. I think we can leave it as it is now and change later if we make changes to the feature.

alright will do so!

ok!

yes it will still work

Here the value of property isn't change to correct format of MB3.5.

Here we also need to fix the property to MB3.5 to make it a meaningful test although it's invalid test.

I'm not sure if this is inconsistent with Code Quality. Maybe we can check it in the Course Website later.

Here the test case need to be implemented.

On my own opnion, this should not be commented right?

Same for this file.

Same problem found^

Here the format of deleteTag example seems not correct? Maybe it should be 'deleteTag 1 t/homework'

It may be better to add more explaination similar to this?

Maybe this kind of command is not added into UserGuide if I'm not wrong?

Nice defensive style code

Have no idea if the empty else block is acceptable in the CS2103T style. In addition, does that mean we will do nothing when the moduleManager doesn't have the existing module?

Similar doubt as above

Will it be bettter to implement a method in ModuleManager to check this? Since the implementation here may violate the Law of Demeter.

Here existingModuleInStr may be a bit confused since I think it means the list of valid module code right? existingModuleInStr seems a bit closer to the modules of current mapping though it's just personal consideration.

Maybe the empty line can be deleted directly? In addition, what if the mapping doesn't contain the module code? If we will never enter the box here we may use an assertion here : )

I see. Then maybe we can show a message that json file is manually modified and the corresponding task is not supported?

I think if we will never go into the else block then we can modify the if-else structure and use assertTrue(mapping.contains(module.toString())) above. Which seems like this:


assertTrue(mapping.contains(module.toString()));

List<Task> newList = mapping.get(module.toString()); //must ensure Module exists in the listOfValidModules

newList.remove(task);

if (newList.isEmpty()) { //remove the module(key) from mapping if no task is associated with it

    mapping.remove(module);

} else {

    mapping.put(module.toString(), newList);

}

Do you think it's a good idea?

sure! I will fix this.

OK!

Yep. I will delete it.

Sure. I will fix it.

I think for this one, may be we can discuss in the next meeting.

OK!

Fixed.

This is because default compareTo will make the task with lowest workload shown at the top of list. But I think the feature may be more useful to show the task with highest workload at first. How do you think about it?

LGTM

LGTM.

LGTM

For LICENSE, there's no need to change person to task.

For README, the AddressBook should not be changed since we are referencing the original.

For this one, I think its best to get rid of this line if we don't have a team email.

I think we should keep addressbook in this file. The link might break and we're still not sure how DevOps will change from here.

Likewise for this one, I'm of the opinion we should keep the addressbook here

I think we should delete the tutorial docs from our repo since they are unlikely to have any purpose in the future.

I think this unused code block has to be removed.

I wouldn't describe sorting as replace task in javadoc.

Personally, I prefer to initialise both index and tags in the constructor. But I'll leave it up to you to decide.

I don't think all the other prefixes (apart from PREFIX_TAG) are necessary under tokenize method.

I think the UG should also mention what happens when the time is not keyed in. E.g. The current time on your computer is taken.

I recommend moving this block of code to before editedTask. Avoids unnecessary generation of the editedTask object if there's no tag to delete.

Not sure why there is a new Use case 9 right before the NFR section.

Nice to see the new commands here. But I think the new mod command for finding by module should be included + the top list of commands and bottom summary should also be updated before the UG update is complete

To add on, maybe the valid module criteria should be listed at the top of the UG, before the instructions. But I think it can be added in later.

Not sure why there's a minus sign here

I see. Sounds good

I have a few questions:

Wouldn't the edit command suffice for changing the recurrence of a task?
If we decide to keep the recur command, is there a way to remove recurrence without using edit command? I think users may be confused as to why the way to remove recurrence is not obvious.

Actually, for 1, scratch the question. We also have a tag command and edit command that can handle tag changes.

To clarify, the module field should be changed to a proper module code?

To clarify, is the deadline here supposed to change?

I assume here the ADDRESS should be changed to DESCRIPTION. Is there any other changes to make?

I was considering using enum but I couldn't get the constructors to work. Maybe I'll look into it in v1.3.

I've changed it to show how many tasks there are across all 3 workload ratings for a single module.

Just to clarify, it's just re-add the original workload count implementation?

So we have:

-One variable counting how many low workload tasks there are

-One for medium workload tasks

-One for high workload tasks

-One that sums up the workload rating translated to int

Pull request closed to avoid accidental merging into the team repo.

Remember to change the prefix to what we discussed

addTask title -d description

Just want to check if there is a need for this class or can we store the description as a String within the Task?

Same question as Description Class. Should we store it as a String instead?

Are we supposed to update the person list?

Also might need to change the comment here, other than this small issue can merge

Should this variable should be renamed to MESSAGE_INVALID_PERSON_DISPLAYED_NAME?

Lets standardise the naming to be XXXMemberCommand to make fixing easier

Think we can rename this file to DeleteMemberCommandTest to standardise the file naming

I think we should make a Util file for the names and import them instead of hardcoding the value here. This makes it easier for maintaining. For example, the TypicalPersons java class in the testutil directory is an example of what I mean

Same issue about hardcoding values

I thought names should not have prefix?

Should we follow the format of the actual phone number -p 95652233 (with spacing)

I think we should follow our prefix convention using -. For e.g. -a

I think this should be changed to invalid name, try including some alphabets mixed with numbers

I think you forgot to change this

I think you might be missing the task status for the Card

I think here we can return the created LocalDate?

Typo?

Close PR due to extra commits

PR Merged. Modifed and Reused Person class. PR #65

Just have 1 small thing to check on, which is the updating of updatedPersonList when we are updating tasks. The rest LGTM

Add task status to edit of task
Updated UG
Add space behind prefixes to enforce spacing

Update UG and Javadocs
Update jar filename and log filename

No longer implementing feature

Renamed addressbook.json to heymatez.json

Not implementing


* `delete -t colleague` deletes the "colleague" tag in contacts that contains this tag. If "colleague" is the only tag that that contact contains, the contact will be deleted.


  * Delete every person that has only 1 tag which is the given tag.

  * If the person is tagged by another tag, only the given tag is removed from the person. The contact will not be deleted.


* `delete -t colleague -t cs2103` deletes the "colleague" tag and "cs2103" tag in contacts that contains these tags. If "colleague" and "cs2103" are the only tags that that contact contains, the contact will be deleted.


            + "OR delete the person if that person has only 1 tag which is the provided tag\n"

            + "OR remove the provided tag from the person if that person has other tags.\n"

Does this mean we should create another command just to handle contact deletion and this delete -t command only deals with tag deletion? Basically similar to our add, find and delete commands that only deal with contacts while tags command only deals with tags.


        return isDone ? "\u2713" : "";


     * Tick represents done, empty string represents not done.

Suggested changes to the javadocs accordingly, thanks!

If I'm not wrong this implementation is the same as the UniquePersonList. Do we want to change? I guess if we change this then we should standardise and change for the UniquePersonList too.

Would it be better for us to display another message if the tag specified is not found in the listed persons?

Currently it is "Removed tag from: ". Maybe we can change it to

eg. "This tag does not exist in the persons listed. No tags were removed."


     * EventDate must contain a year.


 * Parses input arguments and creates a new EEditCommand object


     * Parses the given {@code String} of arguments in the context of the EEditCommand

     * and returns an EEditCommand object for execution.


     * Creates and returns an {@code Event} with the details of {@code eventToEdit}

     * edited with {@code editEventDescriptor}.


     * @param index of the event in the filtered event list to edit


 * Edits the details of an existing event in PartyPlanet.

LGTM

Are we going to throw error if the user types in wrong specifications? like eg. list -partial or list partial or list Bernice. Currently if i'm not wrong it just ignores and lists all people in the addressbook

LGTM

(On a side note: would be good to implement min height for the event later on too)

Currently only allows deletion of 1 tag at a go (only the last specified tag will be deleted).

eg. edit --remove -t friends -t colleagues will remove colleagues tags from all listed persons. Should we allow deletion of multiple tags at a go?

Currently only allows deletion of 1 tag at a go (only the last specified tag will be deleted).

eg. edit --remove -t friends -t colleagues will remove colleagues tags from all listed persons. Should we allow deletion of multiple tags at a go?

Technically the proposal does specify multiple tag removal, but the syntax didn't reflect it :p

If implementing, can use argMultimap.getValues in line 92 of EditCommandParser.java.

If not implementing, might be better if we change the success message to include which tag was removed.

Can possibly also write an error message telling them they can only delete 1 tag at a go

Do we want to allow sorting based on multiple conditions? eg. I have 2 people of the same birthday and I want to sort based on ascending name then descending birthday. If we do not want to allow it then should we throw an error when a person inputs more than 1 -s?

Do we want to allow sorting based on multiple conditions? eg. I have 2 people of the same birthday and I want to sort based on ascending name then descending birthday. If we do not want to allow it then should we throw an error when a person inputs more than 1 -s?

Probably not, since it increases the complexity of the command + the parser doesn't distinguish the absolute positions of the prefixes, so matching two prefixes will be a pain.

Throwing error is one way, and the ~easy way out~ other way would be to simply document this _behavior feature (i.e. only the last sort prefix given is used), just like for the add command. Which do you prefer?

I think documenting is good! Which do you prefer?

Do you think its better to include the --exact and --any flag?

eg.

delete --exact -t a -t b deletes all person with tag a AND b

delete --any -t a -t b deletes all person with tag a OR b

might be easier for users and it is consistent with list and elist

Currently all successful elist actions display "Listed all events" as the result message. Should we specify them? for eg. "Listed all events in chronological order", or "Listed all events in ascending name order".

For this I believe its the same as list. I guess if want can change but must change for both. Maybe I open an issue for changing the output message for both list and elist to do later on? Then if this one no other issues then can merge first.

Should this be edited or removed?

Perhaps the responsibilities for each member should be updated?

should we have an overloaded constructor?

perhaps this method may seem a little too long? maybe we could use a stream here

what is the difference between passenger and commuter?

Is this method meant to be person?

did you mean stub?

i noticed the same potential problem in the rest of the code

small thing but, perhaps this could be renamed getTripDayAsString() as we have discussed

small thing but, perhaps this could be renamed getTripTimeAsString() as we have discussed

perhaps we should call TripDay and TripTime's source.getTripDayAsString() method so as to not violate the law of demeter?

should these be the actual values in the native types?

or should these be the strings, because i believe command test util is to test the input that the user puts in (in the form of a string). To test if it is correct or wrong input

this is ok because its testing the constructor, not the command

this is ok because its testing the .equals method, not the command

i believe this should be ok, because its for the purpose of constructing a passenger

LGTM 👍 Closes #5

LGTM 👍

Resolves issue #57

update price field for add and edit command

Update methods to not violate law of demeter

I like how you used this plusDays() method in your code, didn't realise it exists

Looks well-implemented to me, seems pretty extensible if we need to add more formats or features

Perhaps you could consider using different dates here so there's more variety in the tests?

Looks pretty thorough to me, nice

I'm not sure if this may be necessary, but I think it could be good to have two date strings, parse them, and make sure they're equal in the date value represent? Just to check that the equals method doesn't break

A lot of formats seem to be covered here, nice 👍

Are there any delimiters your delivery date class is intentionally not meant to handle? If so perhaps you can add an invalid delimiter test here

Thanks for the heads up 😃

I think it might be better practice to keep request.value() as private and not public? you could have an isEmpty method in request itself

It might be better for anyone testing/using the product if you use a variety of requests in this file 😃

Think a variety of requests will be good here too

Maybe the Name class contains the regex you're looking for? You could adapt the "\p{Alnum}" in the Name regex to \p{Alpha} . I'm basing that off this link (https://docs.oracle.com/javase/8/docs/api/java/util/regex/Pattern.html)

Also, I think when the parser part is done, it should be trimming any user input before the Type constructor is called so checking for spaces may not be an issue?

Thanks for catching these 💯

Think you mean Cost here

If these two classes are supposed to be the equivalent of CakeCollate.java and ReadCakeCollate.java, maybe this could be better named after the model name? so something like public class OrderItem(s) implements ReadOnlyOrderItems.

Something trivial, but I think it could make more sense while writing the guides, what do you think?

(For your future testing purposes) I wonder if a single " will be accepted? Can be added to tests if you think it's necessary 😃

Got it, changing it to "I can reflect mass cancellations in my database if they occur"

Thanks for the comments! Making all the changes you suggested now.

Rephrased a few things similar to this suggestion and removed all full stops 😃

Good point, added 😃

Oops, thank you, fixed

LGTM

The order prefixes are wrongly written as "/o" instead of "o/"

I think someone can test the example commands given in the UG towards the end of v1.3b to similar errors (perhaps in error messages too)

Missing javadocs comment? Missing test?

I think the javadocs comment doesn't match the purpose of the constructor. please use the other command available as reference and rewrite the comment.

When override, Javadocs will use superclass javadocs when there is no javadocs comment on child. I don't think it necessary to redefine the javadocs comment here.

Missing javadocs comment, Missing class test

Update the test case on DictionoteParserTest? I notice there no test added, please add all the required test for all the method you added, if applicable.

Update ParserUtilTest?

May I ask why does the addressbook store note list? I remember there is a ReadOnlyNote class.

Missing javadocs comment

Update the javadocs comment?

using wrong placeholder

FXML not defined in MainWindow.FXML. please use noteListPlaceholder instead

The stud is used for AddCommand, It is unnecessary to add note method here. In addition, AddNoteCommand test should be done in a separate class.

mayble we should be consistent here, and use week/ header/ maincontent/, or w/ h/ mc/

addnote [c/CONTENT] [t/TAG]... instead of [c/CONTENT]... [t/Tag].. as '...' indicate mulitple

maybe mention what the note is sort by? e.g alphabetically

I have asked the question via the forum, the lecturer said no need to follow java coding standard for fxml file generated via scene builder

https://github.com/nus-cs2103-AY2021S2/forum/issues/195

I have asked the question via the forum, the lecturer said no need to follow java coding standard for fxml file generated via scene builder

https://github.com/nus-cs2103-AY2021S2/forum/issues/195

yap

Should there be a space after female? Not an issue, just a formatting thing really

Just like the one above, the index numbers in the example are missing the c/ prefix

The index in the example lacks the c/ prefix

Should the javadocs be represented like this or would a single line as per the original document be better?

same goes for here as per above

closing as upon discussion, we agreed that it's alright as checkstyle tests have passed it

wrong spelling for passenger

Address app should be changed back to GreenMileageEfforts

closing, resolved on improved code quality

Could you maybe change this to find a/serangoon returns Bernice Yu, David Li instead? thanks!

adjusted the description, closing comment

is this necessary? thinking because you could just use TripTime's internal toString() method implicitly but it could also be that im not too familiar with this

maybe keeping the requireNonNull might be useful since that's what we're encouraged to do this week

could we actually just change this to isSameTrip to keep the code shorter?

Shouldn't these import statements be explicit in order to comply with the Java coding standard?

Shouldn't the length of any line of code be limited to 120 characters maximum?

Does the maximized="true" part starts the program in maximized window mode by default?

Should the code block in lines 18-20 be written inside the constructor in line 22?

Should both paths use the same "\nLocal data file location : " string prefix?

Should this semicolon be here?

Shouldn't the JavaDoc for this method contain a tag for the parameter note? (i.e., @param note >description>)

Oh I see, sorry for the misunderstanding.

Shouldn't it be from the notes list instead?

This issue appears in other parts of the code as well

Shouldn't it be new DeleteNoteCommand object instead?

This issue appears in other parts of the code as well

Shouldn't it be from the list instead?

Shouldn't it be show no note instead?

A minor issue, but shouldn't there be a space before and after and note book and and dictionary content?

Shouldn't this be listnote instead of editnote?

I notice some class may need to be renamed:

PersonUtil -> ContactUtil?

PersonListPanel -> ContactListPanel?

PersonCard -> ContactCard?

JsonAdaptedPerson -> JsonAdaptedContact?

otherwise, LGTM

Right, but those were left unchanged because other Contact commands rely on them.

I will rename them when I update the other Contact commands 👍 .

Just a suggestion. Would it be better if you put everything in a try-catch block?

try {

if (test.matches(VALIDATION_REGEX)) {

    LocalDate.parse(test);

    return true;

}

} catch (DateTimeParseException | NullPointerException ex) {

return false;

}

Personally, I think this is okay since this is within the test folder and we are abstracting the value through key-value mapping. Furthermore, we do not many variables so as long as each JSON object follows the same sequence I am good.

Why did you remove this check?

I think if you can avoid nesting if-else it would be great. So you can do

(test.matches(VALIDATION_REGEX) && value > 0) {

return true;

}

It increases readability as well.

But as long as your Long.parseLong(test) is the second condition to test.matches(VALIDATION_REGEX), it will be fine due to short-circuit.

I assume you use this line of code to debug and forgot to delete it.

I get what you are trying to do here. But your code here seems like you are comparing two items instead of comparing the item's quantity.

For me personally, I feel like this entire code should be abstracted to the item class and name it as quantityPriorityComparison.

What do others think?

Details are in quantityComparator and item class

Same as the quantityComparator, I think this method body should be abstracted to the item class.

What do others think?

Carry on from quantityComparator and ExpiryDateComparator

All this method can be removed and replace with a method like quantityPriorityComparison and expiryDateComparison. It would somewhat be like a compareTo method in the Comparable interface.

You forget to add the throws exception in the javadocs

Agree with kums!

Prevent using magic boolean. Other than that LGTM

Just a suggestion. Would it be better if you abstract the inner if-else clause into another function? Because both the if-else clause looks similar. Maybe, you can have the function takes in another argument to differentiate these 2 different if-else clause?

Just wondering, does removing this test decrease the test coverage? Would it be better if you change it to assertParseSuccess since we are allowing negative numbers now.

All right noted!

Yes, it will work as normal. I am using inheritance to achieve both showing all items when no argument is given and also showing filtered items when arguments are given.

I think it is fine. I follow the AB3 format, but we can always revisit the User guide and developer guide along the way. 😄

Great catch! Thanks! I will change according!

I agree that the message is the same as the above two lines. But, the description doesn't fit my usage, that's why I added a new string. I shall change to a different string description.

All right noted! Will change accordingly.

In my ItemExpiringPredicate Class i use a method called days.between to check for the difference in dates and it returns me a type long.

There are actually no ways of going about this. It's either I parse int to long in this class (which I did) or I parse long to int in my ItemExpiringPredicate Class. Since this class name is called ReminderCommandParser, I feel all the parsing should be done here hence I chose the first method.

@Md-Fazil All right I will try to do so.

@JayChenYJ I personally feel it's not good to allow the user to input "reminder 6 hahahaha" as an acceptable input.

We have to split the string because the parameter can go can be 1 or multiple digits so I just do substring, I have to abstract out the number. With input as "reminder 3 hahahaha", I still have to abstract out the 3 using split.

I feel like prefixes should solely be used to detect each attribute of the items. Whereas this is more of detecting a keyword.

All right! I will make it case-insensitive.

All right! Will change accordingly.

All right will change according!

So I should break into 2 statements?

ExpiryDate expiryDate = item.getExpiryDate();

LocalDate itemExpiryDate = expiryDate.expiryDate;

This wouldn't be the case. It will still work regardless of how many spaces are in between the reminder and the number.

Eg. "Reminder 3" would work the same as "Reminder 3"

Basically, I abstract the user input string into arrays of string. The stringArgsArr[0] has to be a number in a string format and the stringArgsArr[1] has to be a "days" or "weeks". With these 2 valid arguments, I will proceed to parse the number of days in total. If no days or weeks are declared in the user input, by default the number given is in timeUnit days.

All right because initially, the plan we discussed was to make the input more lenient to allow the user to input "reminder 3 days" or "reminder 2 weeks". But I can make it work using prefixes as well, but I probably going to do it on another branch "Modify Reminder Feature" for v1.3

All right!

I made changes to the assertion. It's no longer in this class hence the if clause is still valid.

I added the test back.

All right thanks!

I am just following the format of other use cases.

Correct me if I'm wrong - DisplayFilterPredicate.test() will return true if field is specified, and only these specified fields will be set as visible and managed. If that is the case, can I clarify what you meant by "returns true (...) should be hidden"?

Looks good to me 👍

Just spotted this: "@param displayFilterPredicate that returns true if prefix linked control should be hidden",

looks to be the same mistake as before (ie. returns true if (...) should be shown). Maybe check the other areas where this line was copy-pasted, to see if there are other uncaught ones.

Just spotted this: "@param displayFilterPredicate that returns true if prefix linked control should be hidden",

looks to be the same mistake as before (ie. returns true if (...) should be shown).

Correct me if I'm wrong - this Command class is different from the one in address/logic/commands right? There is one Command class in that path, so I'm not sure if having two classes of the same name would be confusing.

Good point, thanks for the explanation. No problem here then.

Maybe rename the boolean variables to eg. isX, so that the variable name immediately implies a boolean value. In general it is clear that these are boolean values, but perhaps a rename would be safer to comply with the textbook recommendation.

Your suggestion sounds good to me!

I am aware that some of the pre-existing boolean variables from AB3 might not seem to exactly comply with the textbook recommendation, and have thought about just leaving the variable names as is. However, I currently feel that it is better to err on the safe side, as it is always possible for the reviewer to argue that "just because it was originally named this way, doesn't mean it is correct". What is your opinion on this matter?

Nice, LGTM now! Will be marking this conversation as resolved.

Maybe it would be easier for the reader to understand what this line means, by providing an example of how the index of the focused contact can be autofilled. At the moment, it might be difficult to visualize how this feature works (referring to the point on autofilling index).

Slight typo for component

Maybe capitalize Tab for consistency with the other two keys?

Perhaps rename this to better imply that this is a boolean.

Good catch, will be merging after fixing the typo.

Oops, forgotten to include it in my PR description. I provided the reasoning in the relevant commit message, so I'm going to paste it here:

This change fixes bug where parser previously cannot distinguish

between eg. -p and -pp. Now, the parser can differentiate between

-p and -pp , as -p is not a front substring of -pp due to

the whitespace char at the end of -p .

Accordingly, AddressBookParser has been modified such that it only

removes leading whitespaces, and no longer removes trailing

whitespaces. This is to account for optional options such as -r ,

where -r will not be registered as a valid key since the

whitespace char is trimmed away before the parsing of keys.

Not removing trailing whitespaces should not affect the eventual

values of the keys, as tokenizer will still trim the parsed value

before returning it. ie -a test street still results in

the address value being test street, without trailing spaces.

LGTM

LGTM!

I realised that the help text for add command does not include spaces between the flag and the value. I'm guessing the edit command will have this issue too

Thanks for catching this. I'll fix it up in my next PR, after checking the other commands that might be affected.

@oeiyiping will you be updating the diagrams for your changes to Remarks?

Was intending to settle diagrams in 1.4. Unless we are aiming to complete diagrams in this milestone?

After testing the new prefix parsing implemented in this PR, I think there are fundamentally a few problems here, especially with the filter and alias command.

The filter command is having trouble working as intended. For example, if I want to filter by address, I would need to input filter -a with whitespace behind.

Similar, typing filter should clear all existing filters. However, without trimming the trailing whitespaces, the filter command will recognise the command slightly different and instead filter the list of persons to show just the name.

Alias commands are also having problems when parsing and checking the validity of the commands to be aliased.

Thanks for pointing the errors out, I should not have assumed that these cases are already accounted for in the current testcases. I will see how else I can fix this bug while preserving the Bash-like format. Changing this PR to draft until then.

Did you mean to edit all the portfolios?

Did you mean to edit this as well?

Not all commands will be inside. Unless we want to change the implementation

Refer to line 262

Can consider refactor through law of demeter?

Should we delete this instead?

I have edited the help command already in the new PR.

All of the message constrains for commands have a certain format that we use, is that format applicable here?

Is this test case correct?

Did you mean for this month to be valid but the frequency to be invalid?

Should the startTime be made 12:30? Or can it be empty?

Is the javadocs okay in this format?

Because each section of our project has an in-charge (e.g Yongliang is currently in charge of the developer guide), I think we should assign the in-charge guy of that section to review the pull request. It can allow the reviewing of pull reviews to be more spread out among everyone, as opposed to one person doing a majority of the reviewing.

Maybe you should consider having 2 overloaded methods:

public boolean equals(Object other) and public boolean equals(SendCommand other)

but it's ok if u prefer yr current way.

Will be good to follow the spacing format shown in other MESSAGE_USAGE classes for more readability.

I think this extra indentation is not necessary?

The spacing for here too

I think we should discuss more if we should allow duplicate endpoints because other TA/tester may view it as a bug.

Same duplication comment as above

It will be good to include a check that method can only be "POST" or "GET" or "PUT" or "PATCH" or "DELETE", just like how the email checked that it had an email had @ and .com

Else, the method will throw an error

Would these test cases not being changed cause the tests to break down?

oh sorry :>

Just to confirm, Header's regex checks that a header is in the JSON format right?

If not, such deletion before checking may lead to bugs.

@ong6

if a user wants to remove a tag/data, they will most likely type

"edit 1 -t" and then its isn't removing the tag/data if the prefix is "-t "

which makes them think it is a bug / something is wrong (i was stuck on this for quite long)

I checked ab3 there are no space in their prefix so i think just follow that bah

I will resolve in another pr :>

O man

wrongly tried to merge master instead of the respective branches

refactor name to method (-x)

refactor address to url (-u)

I will update the docs for my 2 pull requests in another issue tracker

I did a quick skim of the files and it seems that the whitespace removed from the likes of PREFIX_METHOD are instead being introduced in the strings they are being concatenated with. Am I overlooking anything?

Hi the only thing in the PR that affects code is the prefixs being changed in CliSyntax.java. For the add, edit and run, I just made the error command messages more readable, for example: from

"add -xget -usomeurl -hsomeheader" to

"add -x get -u someurl -h someheader"

Okie @ong6 introduced the whitespace in PREFIX_METHOD in this commit 5 days ago so I think's best for him to do the review.

@ong6 so heres a more in depth view of whats wrong with PREFIX_METHOD = "-x "

using an example of "edit 1 -x "

when passed into the parseIndex method of ParserUtil.java, it calls

string trimmedIndex = oneBasedIndex.trim(); which trims the string to "edit 1 -x".

then because PREFIX_METHOD is "-x " (with the space), StringUtil.isNonZeroUnsignedInteger(trimmedIndex) will be true, because trimmedIndex will be equal to "1 -x", which leads an exception of the EditCommand.MESSAGE_INVALID_INDEX being thrown, instead of a more specific error message from the parseMethod method.

Also, if PREFIX_METHOD = "-x ", doing "edit 1 -t " or "edit 1 -h " does not work to remove tags/header because of the same reasons as stated above.

TLDR: there should be no space in all prefixs, unless you want to change a ton of other codde

@tjtanjin the run and send test lowly unreliable they sometimes pass sometimes fail on my comp and I couldn't even commit my work on sourcetree.

For me, the mainwindow background disappears after I toggle

oh ooopsie


        if (lst.isEmpty() || isDifferentFromLatest(s)) {


    private boolean isDifferentFromLatest(String s) {

This doesn't seem like a very robust check, since it would allow dates such as 2020-99-99, which would be an invalid birthday as well.

I think we could instead use the Java LocalDate class, which provides the parsing methods as well.

You could refer to what I did in my IP, for the deadlines. https://github.com/nickyfoo/ip/blob/master/src/main/java/duke/command/DeadlineCommand.java

or here for usage of LocalDate class

https://www.baeldung.com/java-string-valid-date

It might be best not to catch all exceptions here, perhaps writing the specific ones which might be thrown by the Birthday constructor?


| `**` | Night owl | Enable dark mode | Use the app safely in dark environments |

Reminder to change this to not show an X as it's kind of unsightly

No, we decided that the cross for an undone Event would look very unappealing, and so we've removed it

It seems like the method is doing more than it should, in the sense that it is checking if the person has tags, and adding it to editedPersons, which might violate some abstraction principles.

On that note, the method name is misleading, since it implies that it only checks if there are tags.

I would suggest to change this method signature to private boolean hasTags(Person personToCheck), which will return true if the person has the tags, otherwise false, and leave adding the person to editedPersons to the execute method


* The birthday must be in a valid date format, with or without year, and must be in the past if the year is specified, e.g. 13 Jan, 13 Mar 1997


Can be invoked repeatedly until there is no more history from the current session.


* The date must be in a valid date format with year, e.g. 2022-05-07, 2 feb 2021

Method name is a little misleading, as it seems to imply a check (i.e. returns a boolean). I think this method shouldn't be calling deletedPersons.add(person), as it might violate some abstraction principles.

It might be good to abstract this out into a boolean hasTags() method, that checks these things, and then leaving the adding to deletedPersons to execute.

Thanks, corrected.

Added, thanks

LGTM

INFO: Added state to stateHistory. Current number of states is: 2. Currently on state: 1 is the logged message. Seems like we can have a feature to easily redo as well.

Yup, definitely planning to in a future iteration. I mentioned it in a currently unimplemented nextState method in StateHistory

Ah that's true, will do that instead.

Yup, I think it'll be good to have the help command provide both immediate (summarised) help, and a reference to the link at the bottom for users to explore the nuances

Apologies, it slipped my mind that Birthdays should be sorted by Month and Day instead of including year

The same exception is thrown when tags -f [CHAR] is called, where CHAR is any character not in any tags, e.g.

tags -f z for the sample data.

Are you missing the "task" in your example?

Is it possible to add a constructor with some parameters instead of just an empty class?

Instead of having separate class files for startDate and endDate, it might be better to condense into a single Date class. Same for the startTime and endTime

Is it possible to add a constructor with some parameters instead of just an empty class?

Javadocs not updates here.

DateTimeFormatter and Regex validation format should follow that of the UG: YYYY-MM-DD

I think you mean ".json" here.

right, missed it. will work on this now.

right, but i think instead of storing a null value ill set it to a default priority value

I am not sure whether Deadline should extends Task. Except from that, LGTM.

Might be better to have a single Date class under models/commons instead since the functionality will be around the same since they are all dates.

Adding of constructors to underlying attribute classes delayed.

Added DG updates to the PR:

Created skeleton DG for future edits
Updated AB3 Storage documentation to Sochedule Storage documentation (closes #158)
Added Implementation details of sortTask into DG

Priority and phone has the same prefix, may need to be careful later

I suppose this file is not updated fully yet

are we gonna change the prefix_phone and email all that?

oh ok haha

this should not be imported yet.

are we gonna change the prefix_name and phone and email etc

this may not be an issue but I am thinking maybe we will need something similar for our tests later

ah jiahe I also made this changes, we may have conflicts later

needs because it fails some test case, I suspect storage is not working properly or something

this format follows java syntax, I cant recall the difference hmmm

oh if you are removing it I can add it back

Looks great thanks for the update!

@ljhgab i think this may have been done?

@ljhgab I think this may have been done?

Err..is this space suppose to be there?

I think one more test for "tag does not exist"?

An empty line between description and parameter section. According to CS2103T coding standard.

Similar issues in the rest of code.

Empty line here.

Here

Should there be some test for execute?

Nice suggestion, updated!

Fixed.

This is what I thought and planned to do, but I implement this way is to achieve consistency with editCommand.

Updated

I have changed this to addTwoTags.... In this case, two and multiple have no difference, and adds a set of methods just for this test is a bit.. wasteful?

Updated with javaDoc

Documentation (UG) updated

YEY!

This is done!

Done

Done!

Perhaps this line can be broken down into smaller parts for better code readability?

Perhaps this line can be broken into smaller parts for better code readability?

Maybe this is not needed and can be removed?

Perhaps this can be broken into points for easy reading.

i.e. Reason for current choice:

For the steps in the scenario, perhaps we can standardise to "Step 1", etc. Like edeline's and the undo portion which existed in AB3 before hand.

Thanks for correcting this, totally forgot about this 👍

Would be good if you could separate the export and import into 2 different parts.

Perhaps you could separate the guide for exporting and importing data?

Looks good 👍

Maybe can use StoreMando instead of storemando

Instead of “value”, maybe you can change the variable name to “date”

Maybe you can use StoreMando instead of storemando for consistency

Agreed

Would "expiry date" be better than "expiryDate"?

Maybe it will be clearer to put it as "2nd item of the current list" as user may think that it is the overall list.

"sort quantity asc" is now the updated command for sorting items in ascending order of quantity. There is also the "sort quantity desc" command for sorting items in descending order of quantity. You may want to update this! 😃

Just a minor concern, but does the number of spaces matter? e.g. Will 'Room Living ' match 'Living Room'?

I think both are fine. Since it's just an additional word, I think it's ok to leave it as it is!

Just a minor language error, "is neither day(s) or week(s)"

Please look at the updated version. I have modified the sample names, they are now in alphabetical order.

looks good!

#10

LGTM

looks good

LGTM!

The logic flow should be:

IF patientIndex is provided -> user wants to change patient for the appointment {

patient = patient at patientIndex

} else {

patient = same patient from appointment

}

Javadoc to update

This one should be getAppointmentSchedule instead

Is this supposed to be FindAppointmentCommand.MESSAGE_USAGE instead?

The predicate cannot check for keywords in the UUID.

The variable pt might be misleading here

Should equals check for UUID field?

Is this supposed to be String as per JsonAdaptedPerson

Will remove comment, but in order to test doctors too will require another AddressBook>Doctor> as a parameter in the future

Separated into 2.

Another predicate accepted by the method, NameContainsKeywordsPredicate must be a Predicate>Person>. So cannot change this.

Should this part be reflected via an optional frame, or should the fact that it was successfully executed be an underlying assumption? Maybe the branch here is better explained through an activity diagram, and the sequence diagram should show the "Happy path"? What do you think?

I think there's a typo here, it should be AddressBookParser

It might be helpful to add a note saying that this function does interact with storage, but you've omitted it here for brevity.

Would it be helpful to future readers to show that the isRecursiveKeyword and isReservedKeyword check is also done in this process?

Please remove the comments.

Perhaps it would be helpful here to link to the cascading impact on commands like rdel, redit, odel and oedit, which will need to be modified to block editing until deallocation happens. It would help explain the design decisions made on those commands.

Just a syntactic/clarity thing: Perhaps update the sentence to say "Run dealloc on the resident you are editing first." and then provide an example command

I think the formatting of this should reference AB3: https://se-education.org/addressbook-level3/DeveloperGuide.html#design-consideration

For consistency with other activity diagrams, perhaps state the actual condition in the "happy flow" and "else" for the flow that terminates.

Done, thanks for pointing that out

Done, thanks for pointing that out.

You're right, I have updated it in the commit below.

Agree with Colin. We should discuss this at the next weekly standup.

Beyond that, LGTM.

Thanks for catching this, seems like we all missed it.

I've also added a destructor to show when the AddRoomCommandParser goes out of scope

Rmb to update the java doc!!

Time is not a compulsory field for event right? Only date is if Im not wrong! Is it necessary to throw missing field message format?

Maybe you meant Jackson-friendly adapted "task" object!

in the adapted task

Priority is an optional field?

Rmb to edit the java doc!

Maybe can edit this javadoc or can leave it when removing dependency!!

Rmb to update the javadoc!!

Javadoc not matching the method!! You may wanna update

should be flashcards

need a new line at EoF

Can we turn isNotAns to isAns and flip it inside the method? A bit counter intuitive to read haha

This file (and some other docs) are like no longer needed but for now we just keep them for references bah haha

Perhaps this part can be applied abstraction on? Now the code is getting more and more imperetive haha

should remove this file (user local storage)

I think the .gitignore needs to be modified s.t. data/* won't be version controlled. Previously it was working but now due to some refactoring issues it no longer ignores.

update: (fixed jn)

LGTM but having a separate method like:


public static Flashcard[] getDatabaseOfFlashcards(int numberOfQuestions) {

    return Arrays.copyOfRange(getDatabaseOfFlashcards(), 0, numberofQuestions);

}

is nicer as my pending PR has many tests related to this method so I would imagine there would be lesser conflicts if you have the above instead. Thanks!!!

lgtm if we don't potentially need more booleans for CommandResult (do we?)

should be json array of all tags of one single flashcard

Wrong PR chosen. Sorry.

The weblink for the previously updated README.md is also incorrect (cs2103t -> cs2103 in url, at line 16), please change it as well. Thanks!

Closed #8

This PR will be made as draft until we formally assign tasks for members on 28th Feb.

Reason: to link with issues created after that

The deadline is soft 👀

This is just to 'figure out', so after you know what is going on you can just self-close it or create a draft PR containing the experiment you ran with the code base.

igtm~ probably should merge this first then merge the flashcard class? because classes like phone etc dont exist anymore and not sure what would happen if we merge into that

😮 I will try to merge your flashcard class first; this one is less dependent on other stuff so should be relatively easy.

@xyzhang00 Some simple modifications are needed for README.md as per W7 tasks. お願いします!

I suggest since commands like

AddCommand
ClearCommand
DeleteCommand
EditCommand
FindCommand

are probably not of our concern here, can I try experimenting to remove them? @Jellybeano This will make morphing slightler easier 😃

Added greetings message for the app.

Perhaps create a PR for it, as it will probably be something useful for the application? 😃

Will close it for now, perhaps will reopen and discuss how to properly merge this during team meeting.

Will close it for now. Perhaps will discuss later how to merge it during team meeting.

@xinweit after I merged my previous PR into the code base there seems to be a conflict. The conflict is in MainWindow.java and it seems its relatively simple to resolve from your side. Its a bit troublesome from my side as I would need to make changes of your PR locally, so can help?

You can resolve it by trying to pull the latest master branch to your GUI-display branch. Thx!! 😃

Just clarify: is it clear now where can this Mode class be potentially useful (i.e. help change the mode of display)?

GJ!!

With this commit the GUI now magically can differentiate between learn mode and quiz mode display. But the modification here is only tentative (and far from elegant) and mainly to provide some idea of how GUI works; to further support the next and check feature, more work needs to be done.

Looks like this PR shares a common commit history with previous one, is this intended?

Err so this PR is a super set of your PR 74 and 76? Sorry I am a bit confused

lgtm!

...

Isn't toTest.isBefore(acceptableDate) sufficient haha? Because dateToday is before the acceptable date? And Also because remind is for upcoming deadlines right, you could consider making sure all the dates are after today. In case a person/order with a past delivery date is still in the system.

What exactly does "in the context of the FindCommand" mean?

Wait so whenever the input is an add command the request is initialised to "Give me more pineapples."?

Because you implemented request seperately and not in the add command, do you need the request in the addCommand parse function?

Request only takes in the request field right? So does it need a prefix? Because it will always be r/ and never anything else.

Maybe you can use toString() here instead of value

So the request is like a tag?

ohh icic!

Do you need both isSameOrderItem and equals? You could consider using the equals method only because the same order will probably have the same cost also.

Is there any reason why it should only contain alphabets? Maybe can relax it a bit for types like 12" chocolate cake or something haha

There is no way two help cards will be the same because there are only a few fixed set of commands which are fixed throughout the program which is why I mentioned it. But no harm leaving it.

Okay!

Originally when I switched from the help page to main, I used fillPersonListPanel() in the resetMainWindow() function to fill in the vBox with the person list again, so as I was using it twice I abstracted it out. I changed it to re-using the same person list so I don't need it anymore but forgot to take out the abstraction. Do you think ts better to insert a new person list using fillPersonListPanel() or reuse the old one?

yup

Okay will change all the person and address book references you've pointed out

Lmao okok

Yup, I originally used the function to use from HelpCommandPanel to change from the help to person list there but it was quite inefficient to get everything from there so I ended up just sending a reference to the main window itself to help which is a cyclic dependency now that I think about it, oops probably have to change stuff then.

Hmm the data can change because you can add orders when you are in the help page, you just cannot see them in the panel, so I think that is a better idea too thanks

Oh really? Did not know that haha

I tried using a static method but it cannot access non-static variables so that does not work either

I guess I'll try the association class then, or leave it if I cannot lolol

The reason for updating the endpoint card later is because?

A possible future refactor could be moving the animation to CSS and have it triggered at the start of execution and end when the result returns. But this is decent at this stage.

Small typo in the comment, should be a post request

Perhaps timeout could be extracted out as a static constant to avoid magic number issues?

perhaps can consider using an actual valid url? This example seems fake.

I suppose the comment is outdated? as it refers to phone, email and address?

is MainStreet a valid address?

the comment is wrong?

Is it necessary? because is Address.ValidAddress already invokes isUrlValid method within in it

Alright will remove the two

Looks good! 😄 Just a small note, the image and portfolio name needs to match the github handle.

Thanks! Didn't realize this is required. I have renamed accordingly.

@tjtanjin When rendered as HTML it looks like this:

@tjtanjin FYI I will experiment with this for an hour or two now. If anything I will update this thread.

-update1

Use of Platform.runLater(new Runnable() { ... can schedule the execution and update of GUI to a later stage. But right now there is still some lack before displaying Running test and it does not seem to return immediately, which means the place to invoke the call seems to be important and needs to experiment more on this.

LGTM 😃 Just a trivial point that the comment for endpoint attribute in CommandResult is the same as isApiResponse.

Good catch. Fixed and will now merge.

Great job, while you are at it, could you reduce the 5-second delay that emulates the wait time of an API response 🥺

Curious:

how did you fix the lag?

if we are disabling the user's ability to input while running the request, wondering if it will be better if we still show what he entered in the command box and clear it after the request is done? Not sure if this is the case already... for discussion only

I'll make a PR really soon to remove the wait time :3

Shifting Platform.runLater above the sleep code removed the lag (which came from the initial 1 second sleep)

Yup the entered command stays in the command box when frozen 😄

Alright cool, I guess it's the sequence of calls that messed up the thing.

Damn, I am eating my own poison... will fix this when I am home using my desktop...

Sample endpoint 3 periodically returns an empty result with the following error:

"JavaFX Application Thread" java.lang.StackOverflowError

There is no known way for the user to directly cause it as of the time of writing this. It seems completely random. What is worth pointing out is that sample endpoint 3 returns a very long json result which is also reflected in the extra 1 second it takes to load the result into display.

This seems to be a performance issue and we should consider how to address this as soon as possible.

Have tried a few times and wasn't able trigger it, if it is a peculiarity with regards to this API, perhaps we should remove it as of now.

interface as in the screen?

I would prefer to remove it i.e. maximize the space by expanding the result display. This is to keep the UI simple and more consistent by ignoring screen size differences, which might be error-prone because we need to cater to various sizes. Just my 2 cents.

I think this is a great suggestion which I suppose you have specified in #203, let's put it in 1.2b or future milestones...

Given that we can also retrieve the status phrase from response.getStatusLine().getReasonPhrase(), perhaps we can have a hover over effect that when users hover over the status, they can see the textual response phrase.

Perhaps we could try it out and determine if it makes sense visually. My hope is that it should not be displayed in such a way that clutter the essential information that we are trying to show.

If the length of response is a concern, perhaps we can check the response length and limit that to a reasonable number such that we display a proper error message when the length exceeds that.

I think this might be a UI tweak/task where we explore better ways to the layout of the response(both what we extracted out and the actual response entity-body of the API request). Maybe anyone who has thoughts of how our UI could be improved can have a go at this?

Let's start adding required tests as issues to be fixed by 1.2b so that we don't ignore them anymore:)

Ok I think I need to see how find command works first

Seems like it's fixed, @ong6 can verify again.

@tjtanjin the run and send test lowly unreliable they sometimes pass sometimes fail on my comp and I couldn't even commit my work on sourcetree.

Yes @JulietTeoh if run & send have issues, please include in the error message so that I can take a look 👁️

A link to a video on RESTful API is included in help window by #300

I agree with your observation, though instead of removing the menu bar outright, I feel that we can replace it with a banner saying History and Response, much like your original UI mockup. We can discuss this tomorrow along with other UI tweaks we can make this coming week.

Great suggestion, let see what others think tmr

Codecov Report

Merging #327 (0e3a546) into master (a276db4) will decrease coverage by 0.07%.

The diff coverage is 100.00%.

@@ Coverage Diff @@

master #327 +/-

============================================

Coverage 67.99% 67.91% -0.08%

Complexity 557 554 -3

============================================

Files 100 100

Lines 2037 2032 -5

Branches 202 201 -1

============================================

Hits 1385 1380 -5

Misses 588 588

Partials 64 64

Impacted Files Coverage Δ Complexity Δ

...n/java/seedu/us/among/model/endpoint/Endpoint.java 96.51% <ø> (-0.20%) 23.00 <0.00> (-3.00)

...ava/seedu/us/among/logic/commands/EditCommand.java 96.15% <100.00%> (ø) 12.00 <0.00> (ø)

...du/us/among/model/endpoint/UniqueEndpointList.java 92.85% <100.00%> (ø) 21.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more

Δ = absolute <relative> (impact), ø = not affected, ? = missing data

Powered by Codecov. Last update a276db4...0e3a546. Read the comment docs.

Dude I deleted a bunch of lines and coverage decreased?

You can actually use the Equals method in this same class since the equals method checks for if method, address and tags are the same.

nice catch on the naming here

Since we are not done with EditCommand yet, perhaps we should keep this as a to-do since when we change EditCommand it may lead to some more errors here.

the space there is actually on purpose so that if you enter the commands, there has to be a space else it won't work. Unless u wanna allow them to do -xmethod

wow cool diagram

The grind for comments is real

nice addition.

why did you change this to 0?

Ohh, ok nice

Don't worry, I do not allow duplicate endpoints, the check is as follows -> if the method and address same it is considered as a duplicate.

newline indentation must be 2 tabs wide, some checkstyle error flagged it so no choice

Once again, duplicate endpoints have been considered to issues solved

no, i got rid of all the tests using typicalendpointlist cause idk what they are for

yea, keep this in here.

nice

As the QA for my company, I would like to organize my tasks so that I can identify what needs to be done next

Yay

I agree with yong liang

Don't remove the List command, it shows a complete list of all the API's after the find command. There is no way to do that after the find command is run

Okie @ong6 introduced the whitespace in PREFIX_METHOD in this commit 5 days ago so I think's best for him to do the review.

@ong6 so heres a more in depth view of whats wrong with PREFIX_METHOD = "-x "

using an example of "edit 1 -x "

when passed into the parseIndex method of ParserUtil.java, it calls

string trimmedIndex = oneBasedIndex.trim(); which trims the string to "edit 1 -x".

then because PREFIX_METHOD is "-x " (with the space), StringUtil.isNonZeroUnsignedInteger(trimmedIndex) will be true, because trimmedIndex will be equal to "1 -x", which leads an exception of the EditCommand.MESSAGE_INVALID_INDEX being thrown, instead of a more specific error message from the parseMethod method.

Also, if PREFIX_METHOD = "-x ", doing "edit 1 -t " or "edit 1 -h " does not work to remove tags/header because of the same reasons as stated above.

TLDR: there should be no space in all prefixs, unless you want to change a ton of other codde

Nice spot!

But then this would allow users to do edit 1 -tdog (without space), I guess it would be possible to remedy this somewhere else, but if I'm not wrong, the original AB3 code came with a "/t " (with space) so when changing to "-t " I kept the space as well.

However, after checking out your commit, I think that the problems that the space brings are substantial so this is a good change overall!

This is used in line 20 to ensure validity of quantity

Will be adding test cases for empty String

Any reason why this has to be public?

Not sure if I misunderstand this, but shouldn't this be for orders?

Likewise , please refer to the next comment

maybe we should rename this to be more a more boolean name?

Don't think its a good idea to have method name similar to the field name

Is the original set equals not working? I think the original equals for set do checks the individual elements according to the API.

Sure. Merge his PR and then i will resolve the conflict and add in the missing actions

Noted.

Yeap , will merge his changes into my branch after his PR has been merged

Will update in the next commit

Yeap but even if dont have it's by default false.

Will add it in the next commit.

Could you elaborate?

I was thinking of changing it to status : (Assigned / Not Assigned)

Don't think we should do a counter, as I was thinking of setting quantity's value to be private.

setCheese , yes will be moving it out. realized that I might have not considered an edge case

Sure. will be adding this in the next commit

Will be making this change in the next commit

Hmmm based on the doc , it should work but when i was debugging that was giving me some problems. Will look into it again.

This should be at heading level 3 to be consistent with the document.

This should be in the quotes to match the other "examples" you changed.

Should this be in a different format to match the other "examples"? Additionally, perhaps the newline is not appearing as intended?

Should price be Optional>Price>, similar to driver?

STUD->STUB

Perhaps this chunk could be simply passenger.getDriver.map(x -> x.equals(driver)).orElse(false)

This should be in the constructor

NullPointerException is probably the wrong exception to throw

Fixed in commit 1c344c7.

Fixed by commit 0c1b0db on PR #78

Hi @vvan-essa, I believe you have the wrong team repo, this is for W10-1 (i.e. G01 - Team 1). Perhaps you can check your team here.

LGTM

Closed by #31

Closes #43

Closed by #41

Closed by #73.

Closes #59.

Closed by #76.

Closes #58.

Closed by #60

Removed most traces of AB3. Remaining traces will be left as-is.

Closed by #103

Great job, Good formatting

Good value proposition

great job!

great job

Great job, follows SLAP everywhere!

Good, follows SLAP

Great Job, SLAP used

SLAP-tastic!

Merging to main, DG should update for everyone

ignore this

implemented help command

implemented delete command

implemented edit command

change to "different predicate" line 37

change to "different command" line 53

could add a few more tests just to be safe, for both failure and success, maybe something like "ModuleTutorial" for user input and predicate is {"Module", "Tutorial"}?

change the comment to "// missing description prefix "

change comment to "invalid description"

Following the comment, the assertParseFailure() in line 115 should have 2 invalid values. Can change either the description or date to an invalid value

similar to a previous comment, should change the description to an invalid one

i think there should be a test for parsing with invalid args

ParseExceptions are thrown in line 59 and line 64 for unexpected user input, my assertion in line 57 was mainly to warn whoever make changes to our code against using this public method. It should only be called if the user's argument input passes #hasMultipleValidIndex first, as seen in line 31 of DeleteMultipleCommandParser and line 59 of TaskifyParser.

Sorry, can I clarify which line you are referring to?

Close #9

I think it is. However, you will need to use the exit command so that the storage saved the latest updated task list

I'm not entirely sure, but i dont think we need to use exit command? For the test, can't we just instantiate a new TaskifyStub and then check if its tasks are sorted?

could add a few more tests for parsing using TagSearchCommandParser. Have a few additional comments too

By additional comments, do you mean comments to explain the tests?

I'm referring to changing the comments for line 37 of TagContainsKeywordsPredicateTest and line 53 of TagSearchCommandTest (just the first two reviews for this PR)

Duplicated issue

Maybe assign Set>Tag> tags = firstTask.getTags(), to make code more readable. But not a big issue.

LGTM!

It might be more "visual" to write this in a few more lines. But no big deal lgtm.

Perhaps it'd be better to use variables and methods to decide the size of String[] expectedTags, since "multiple" doesn't always mean 2.

Looking neat!

nice defensive programming

maybe medium and high's first letter should be capitalized like Low?

cool! Wld be good too if u can include a total workload count for each module? Thought that'd be a nice thing to have.

Okay will do 😃

Yeah u right, I think maybe git pull doesn't record the changes properly?

oops

I thought the initial checkstyle rules stated that a space is needed before the brackets

sure!

will implement after we all agree on this new feature in meeting 😃

resolved 😃

yea haven't added. Resolved now 👍

haha

yeah. This will only happen when u manually edit the json file with module codes which are not supported. So the tasks with invalid module codes will just be ignored

Agree! Will resolve now

All resolved!

Yep! Resolved 😃

ah right!

resolved 😃

yep all resolved 😃

hmm but what do we assert here? As in assert(?)

Ah.. I see. I think it works let me change it

Close PR to prevent merging into team repo

Merge to include image in docs/images

lgtm!

Resolve merge conflicts.

I also commented out a chunk of code for 2 tests, pls ignore, I will either add or remove them according to our next discussion 😃

lgtm

Do we keep this? Remarks isn't in our milestone but I suppose we could

Should this also be parseStudentCommand?

getAddStudentCommand

also just curious what is k

Should we use the studentIndex-Session index here too?

Actually there's no use of this toString() method currently, right

Add session to sth?

I guess also do the folder with your name

Coming up, will have to edit this to serve Recurring Sessions as well.

TODO for me

ooh sorry yeah let me just undo the change i simply refactored

Right totally forgot. Will do so in coming commit

No man trying to solve merge conflicts w copy and paste led to a disaster

Will do so in next commit. when plantUML comes alive

No it's not intended because I didn't (and kind of currently dont) know what problems this may cause.

Not intended as in never questioned it

What problems might this cause/ what recommended way might there be?

Was thinking since its extending Session, the parent's method to access Sesssion contents are inherited

no idea myself leemme delete

right!

yep!

Yep thanks. actualy given your comments I think there was also no need for this to be a static method.

Will replace with a .equalTime(SessionDate sessionDate)

yep will do this

Thanks, did in the coming commit

Right ive got to make a RecurringSession json. to save things into if Session instanceOf RecurringSession.

I don't mean readme. i mean't the aboutUs page

There seem to be missing the switch case for delete_session in AddressBookParser.java, which makes the command inexecutable.

I added the switch case myself locally and tried to execute the command when the student has no sessions, seems to throw an exception, IndexOutOfBoundsException, which was out of the application. Maybe you could test it and let me know if you face the same issue too?

Other than the above, the rest looks good! thanks!

I think I've fixed it.

Still have issues with building TypicalStudent with StudentBuilder.addSession(). Which is why testCases are commented out.But this at the moment is another matter.

Would it be better to integrate the Session class diagram in the Model component? I think it would add more value there instead of being a standalone. (We can show the association with the Student class there)

Also, would it be better to make each feature distinct instead of combining it into "Session" feature to preserve the current formatting?

You are probably right about the Model component; it would be good to be together with Student. I think i'd take SessionClassDiagram out of the DG for now.

About the feature:

I did this because creation then a deletion does not each sound like a different feature (if feature is the word used) rather only difference in the command, which may not even need 2 seq diagrams like there already is, i felt. we shld discuss

List>Appointment> should be replaced with ObservableList>Appointment>

You can create a MESSAGE_INVALID_APPOINTMENT_DISPLAYED_INDEX to replace MESSAGE_INVALID_PATIENT_DISPLAYED_INDEX here.

This should be replaced with

'(patient.equals(toCheck.getPatient())

            || doctor.equals(toCheck.getDoctor()))

            && getTimeslot().hasOverlap(toCheck.getTimeslot());'

docs to update to EditAppointmentCommand.

I don't think this is needed since it functions the same as hasAppointment method.

I don't think this is needed as it functions the same as contains.

I don't think this is needed as it functions the same as hasConflict.

Perhaps you could condense this to the same line

This validation allows invalid dates like 99-99-2021. Perhaps you could check it with DateTime classes or use #41 dateUtil?

This may not work for stuff like tar.gz

This might be an extra space

Move these to FileUtils?

Inconsistent spacing between command format and >br>...

Line 286 should be: Advanced users are welcome to update the data directly by making edits to these files.

Was this always there as a duplicate since the initial commit?

Missing Equality Check test


command = 1 -&gt; false

command = null -&gt; false

command = new command of same parameter -&gt; true

command = same command -&gt; true

command = different command -&gt; false

Missing checks


predicate = 1 -&gt; false

predicate = null -&gt; false

predicate = same predicate object -&gt; true

You missed the Same object check

Hmm not sure why test coverage missed it but I guess its fine.

Do either of the following

Sort the collection again in the test case and compare element by element
Use the string comparator to compare every element with its subsequent element in a for loop

The test is to check lexicographical.

If the add command is renamed and no command starts with A, this test fails incorrectly.
If the add command remains but the list is [add, find, alias, edit, delete], this test passes incorrectly.

You seem to have misunderstood my approach. This is not testing the sorting order of your getAutocompleteCommands. This is testing the sorting order of the Collections.sort function.

What you should be doing is

get a completely clean list of command unrelated to your autocomplete command.
Sort that list of commands as per your expected sorting order Collections.sort(cleanlist)
Iterate through this clean sorted list and compare against your test list

This existing test will fail to achieve its purpose when

logic.getAutocompleteCommand("") returns [add, clear, alias, find, delete]
Collections.sort sorts it into the correct order
asserts verify that its the correct order.
Note that the command actually returned a bad order but all the asserts passed.

Fixed in https://github.com/AY2021S2-CS2103T-T12-3/tp/pull/76/commits/c6feea1138bcaae29f8cbfd1591ee31680590f7a

closed with #39 #40 #37 #38 #41

closed as duplicate of #2

Depends on #85

Depends on #84

Depends on #85

Avoid using wildcard import, it would be better to list all the import explicitly

Should move this to the default branch of the switch statement.

You can add which methods you implement for this stats command, you can refer to sort and undo/redo implementation for more details "It implements the following operations:" part.

>div markdown="span" class="alert alert-info">ℹ️ Note: The lifeline for StatsCommandParser should end at the destroy marker (X) but due to a limitation of PlantUML, the lifeline reaches the end of diagram.

>/div>

Add this note under the sequence diagram to indicate the limitation of PlantUML

I'm unable to see the image, you might want to check the link again

Remember to add these 2 commands in the table of content.

You can consider enlarge the image a little since I find the text in your sequence diagram is a bit small.

This update is no longer required as it was changed in another pull request, can revert to the current master revision.

Sorry I think there was some miscommunication as I also updated the user stories! When merging you can see if you want to keep your version or mine, your version looks good as well.

Is this use case not completed yet?

Is this precondition required? I think it is fair to assume that a list always exists and it might just be empty.

Can move to either the parent package model or to a new package model.appointment

Sorry small change needed here, I made the AppointmentDateTime parsing follow more closely with the user guide so an AM/PM is required now. Setting to 12:00AM instead of 00:00 should work.

Please restrict pull requests to only the required changes. This pull request includes tutorial code as well as other code updates which is inappropriate for updating the developer guide.

While I really appreciate the creation of the new class AppointmentBook and the other changes, I must warn against these kinds of monolithic PRs. At the current moment there are too many changes to look through and accept, and I am unsure how some of these changes will affect other aspects of the code. I worry that with the amount of changes made we may be unable to fix all the test cases by this week's deadline.

May I suggest splitting this PR by creating new branches and cherry picking the relevant commits?

Duplicate, resolved by #126

Maybe can create a constant for DateTimeFormatter, same for the output to string

Some notes are bold, while others ain't. I think it would be better if it was consistent

Same comment as above.

maybe using toEditIndex makes its purpose more obv?

Good use of constants!

maybe referencing what the booleans are used for would make them clearer?

Complete UiMockup image

Solved in PR #28

To delete: Address, Email, and Phone

Delete Address #63

Delete Email #64

#67 Add Exam class

#85 Add ExamList

for assignment and assignment list

add, edit, clear, find, delete, done

I can do clear, find and generaleventlist

Done for modules and persons

Clear event too

I can do clear, find and generaleventlist

clear, find and generaleventlist are done.

find is based on description

I think to follow the java coding standard, this should be changed back to listing imported class explicitly instead of the star import.

Same thing here, should probably import the classes explicitly.

Good work. Good use of SLAP.

Thank you for changing to the correct link. Same for the others.

Thank you for updating the diagram.

Good job on the documentation of implementing review feature.

It is okay, the documentation will be reflected as step 1 and step 2 correctly.

It is okay, the user will not be able to search by multiple criteria, it will be either by question or by category.

Thank you for pointing that out. I have changed it.

Yes, thank you for the suggestion. I have tried changing the sequence diagram accordingly.

can you replace lines 1-2 with these


---

layout: page

title: User Guide

I think this is needed for the website to display UG and DG links and you removed these lines in the previous PR.

Currently, if you check the netlify build for the website, there are no links to the UG and DG

Could you update the user stories to show the edit status and sort by deadline stories. Also, I think all the priorities should be High or Medium for the current user stories

I think exceptions are more suitable as we are trying to figure out errors with user input

Should an exception be used here instead?

I was commenting on your comment on line 63 haha

Ok makes sense

Ok, will add! The tests I wrote so far follow the same format as the ones for the find command.

The tests in TagSearchCommandParserTest checks if the parser splits on space correctly. The test you mentioned should be part of TagContainsKeywordsPredicateTest. I have added it there.

could add a few more tests for parsing using TagSearchCommandParser. Have a few additional comments too

By additional comments, do you mean comments to explain the tests?

could add a few more tests for parsing using TagSearchCommandParser. Have a few additional comments too

By additional comments, do you mean comments to explain the tests?

I'm referring to changing the comments for line 37 of TagContainsKeywordsPredicateTest and line 53 of TagSearchCommandTest (just the first two reviews for this PR)

ohh okay 👍

would be good to test if the list of tasks is sorted after reopening the app

I think it is. However, you will need to use the exit command so that the storage saved the latest updated task list

If its too much trouble to use the exit function then just writing unit tests to see if the sorting logic works correctly would be good.

Closes #49

Closes #52

Is there an extra 'the' in the second sentence; 'determines that the the command called is...'

Should the 'ParseException' be formatted as code i.e. enclosed by ``

Should AddressBook be StudentBook?

Do we wanna indicate the multiplicity here? Is it 1-1?

Correct me if I'm wrong, does this mean a student can have 0 or many school residences? Shouldn't it be only 1 school residence for every student?

Have you not merged with the latest master branch, to reflect the new DG?

What do you think if we remove this line?

https://github.com/AY2021S2-CS2103T-W10-4/tp/blob/15c156a02d6463b9dae9bac73a8e90628f763229/docs/diagrams/AddSequenceDiagram.puml#L43

I thought more about this and wonder if this is a self-invocation, which entails calling another method of its own instance. If we remove this line then it would be that AddressBook is activated by the method call addPerson() from Model, then returns and gets deactivated.

Also can you update the image in DG to match the new sequence diagram?

Should this be 'Tutor Tracker'?

(minor) Will removing 'I can' be better?

should it be 'deletes the application'?

Tutor Tracker

should Set>Tag> be TagList?

Thank you. This part is deleted.

Thanks guys! I'll merge this PR first in order to continue adding the command and I'll make the changes based on Vinleon's suggestion later:)

Remember to clean up the comments to match Residence instead of Person

Address Book comments need to be changed to Residence Tracker too

Class name has to be UniqueBookingList I believe?

I think you meant .isSameResidence here?

do you mean seedu.address.model.booking.Booking? I think you might have forgotten to refactor the person package to 'booking' instead?

Once you refactor, you probably don't need this import statement since the Booking is also in the same package and you can use it directly in this file


* At the most recent input, pressing `Down` arrow key once more clears the text box.


* Pressing `Up` arrow key in the text input panel reverts to earlier input.


            + PREFIX_BIRTHDAY + " 1999-06-01 "

Will writing "You can write anything in remarks, " be more easily understood?

This might sound a little bit ambiguous, as the user may confuse it as the person he wants to edit does not have an empty remark. Maybe can change it to something like “You have already typed something!” ?

That's true. I considered LocalDate but wanted to keep to standardising using isValidBirthday boolean to check for validity. Will try to see if I can use both, else will switch to LocalDate. Thanks!

Used LocalDate to check for validity in latest commit.

Separately caught IllegalArgumentException (for invalid year) and DateTimeException (for invalid format) in latest commit.

Adopted in latest commit.

Will leave it to after append is implemented to see how they fit.

This method is following the displayPersons() method in all the delete commands to standardise. Should we change it for all occurrences?

Adopted in latest commit.

Adopted in latest commit

Good catch! Have edited the implementation in latest commit, changed the method name to hasTags that returns a boolean and leaves the addition to execute.

LGTM

The year can be useful to remember a friend's age though, so you won't eg. accidentally wish your friend. "Happy 21st" when it's their 22nd.

Some ideas (screenshots from online):

I think it can be done for the filtered list, so the user can subset the group he wants to delete the tag from.

Maybe make the new theme a separate file and keep darktheme (for a possible future enhancement to let user switch between themes

Oyea good idea! Have adopted in latest commit

When the user input invalid parameters after a flag, the result is the same as elist and also displays a "Listed all events" success message. Optional though can consider to display another message to remind them about the missing field (can be error or just a message).

eg. elist --any sdjsjhf, or elist --exact xcvxg

(Actually this behaviour is also in list for persons haha can leave it or do "for fun" next time if want)

delete without parameters is the intended replacement for clear and edelete works to clear events list.

((sorry i forgot!!)

Perhaps this can be named AddVenueCommand?

Perhaps throwing VenueNotFoundException would be better?

I think CommandException is correct for commands.

Is there an issue with modifying the list while streaming it?

Use removeIf instead possibly

Perhaps abstract the 3 into a variable to avoid magic numbers

This seems different from the ab3 alternative format provided. Maybe using the provided format would be better? Do let me know if I am mistaken.

The referenced link is here: https://se-education.org/addressbook-level3/DeveloperGuide.html#design-consideration

Perhaps the details not choosing the alternative could be added to be clearer why it was not used in the end

This is a small one but maybe add a space between valid and '''

Maybe reword the second sentence as it feels a little weird to read.

The inconsistency is due to the need to distinguish room.RoomNumber from issue.RoomNumber

I would like to discuss if the command history should also include failed commands. I think there is merit to mimic the same behaviour where the invalid command can be access via up and down arrow keys so that a Power User can edit the invalid command rather than retype it all.

If we do go down this path, I think a possible implementation is that each history entry will keep a field to keep the corresponding command used. Thus, if the field is null, it indicates an invalid command, otherwise it indicates a valid command.

I would like to discuss if the command history should also include failed commands. I think there is merit to mimic the same behaviour where the invalid command can be access via up and down arrow keys so that a Power User can edit the invalid command rather than retype it all.

If we do go down this path, I think a possible implementation is that each history entry will keep a field to keep the corresponding command used. Thus, if the field is null, it indicates an invalid command, otherwise it indicates a valid command.

Hmm, thanks for the suggestion. SunRez currently does not consume the user's input when a command is invalid, so a power user can already edit an invalid command easily.

I see, that makes sense. In that case, the suggested feature seems to be not needed.


**View Tutor** | `view_tutor INDEX`, <br> e.g. `view_tutor 1`

I don't think the variable appointmentList and its respective methods are required in the Appointment class, since Yuting is handling the List classes, which already have an AppointmentList.

This line may not be required, as referenced to #12, Tag is not a superclass of Name, Email and Gender.

I think we might need another dateTime variable here because from what I see, the tutee can only edit either the timeFrom or timeTo. If we update one of them, the another's value will be overridden due to how updatedTimeFrom and updatedTimeTo is instantiated. Maybe @kingsleykuan can double confirm on this? Thanks! The rest of the codes LGTM!

This should be EditBudgetCommand instead?

Noted! Will omit during the merge.

This meant to serve as a base template as I modified the styling slightly. Can choose to adopt this styling or go with the default style from AB3.

Noted, I have removed it & committed.

Resolved the merge conflicts and used the Ui image from the master branch instead.

Will resolve the conflicts after PR #2 is approved due to accidental merging of branches.

Noted @kingsleykuan, will proceed to merge and remove the stubs in a followup PR instead since there shouldn't be any interference with the rest of the codes.

In meantime, please also try to fix the build error (I think it's located in the test cases) as we have turned back on the blocking of merge if there's any build error.

Hi @wangtao0717 I believe you should change the filename to your github_username_in_lower_case as specified on the website. Otherwise this LGTM!

Hi @wangtao0717 very minor typo here, will approve when this is fixed!

@awzhenyi Very minor but there is a typo (double space) between 'the' and 'user'. Otherwise, LGTM!

@jaredtengsw minor but apartment should be changed to residence here!

Apartments should be changed to residences

@VRSoorya So booking should be its own package under model? Because I recall someone saying booking should be under residence, but I can have it in its own package if that works better

Fixes #29

LGTM! @wangtao0717 should this say 'fixes #45' or are there more changes to be made?

Addressed by #110

The string "null" could be "no_url" as the insurance agent may not know what is a null when checking the json file.

The log message could be more appropriate.

The code here does not capture the error where user input "lock" has no password.

Noted thanks for the review

Maybe something like clear appointment and clear property? As well as a clear all to clear everything at once?

It seems weird to me that next() for Completion returns a Completion?

Since it implements Status it needs to have a next(), and Completion shouldn't lead to any new statuses anyway, so I guess it's fine to return itself? Creating new objects at a terminal step may lead to some undesirable side effects. By checking if .next() returns the same object it can even be used to check if something is calling .next() on a Completion.

Very minor nitpick, but n/ here should be referring to the property name right? Would it be good to use a property name for the example instead?

Will the user be able to change back to DarkTheme?

i think according to the validation regex price can have cents. Should we still consider that for the price?

The and here is referring to the logical AND, I think I made it quite ambiguous here. Will go change it.

Wasn't thinking straight when writing this. Somehow thought this part will be combined with appointments in the future.

For completeness I guess, maybe can remove.

The keywords have no prefixes though, so I think a for loop like this would be more convenient.

I don't think any property prices bother including the cents part though, other than perhaps after tax calculations? But anyway will go add another field.

Doing that causes quite a number of tests to fail due to having hardcoded comparisons. Since most tests don't actually interact with Client or AskingPrice, it is safer to just implement a new method for properties with clients.

changed to dollars + cents.

I think I meant to say if the user is searching for bob, the command will search both lists for properties with client bob and appointments with Meet bob. I'm using name here since it searches for the string in the name attribute of appointments.

added 313 to 315

Need to decide on how to store housing type first before this can be implemented.

Add github actions badge
Add acknowledgement

Add use cases for everyone's own parts.

Closed as there are changes made to the branch.

15 March discussion:

Limited set of tags
Housing types
Currently not under tag but under property type
Reuse the code from addressbook
Add extra tags: bedroom and bathroom numbers, both existing as compulsary tags

What do the others think of this refactoring of PocketEstateParser?

Looks fine to me, passing checks means the other functions are proabably not affected much anyway.

closed with #167

Good change! The addition of new lines after each component of the flashcard will help to improve readability.

Should this be "This card already exists in FlashBack." instead?

Should this be step 2 instead of step 1?

Perhaps you should also state that the user can search by specifying both the question and category. eg. "find q/ >keyword> c/ >keyword>".

Should we list out all the imports instead of using a wildcard?

Great suggestion. I have updated the section accordingly.

Fixed. It should show now.

Added. Also fixed the broken links in the table of content.

Updated, thanks for mentioning!

LGTM!

Good job! Code is written in high quality with evident use of OOP and abstraction.

LGTM! Tests are well written and have a good coverage of the Find command.

LGTM! Code is concise and easy to understand.

Looks good! Documentation is detailed and well written.

Do you think we need an exam list as well since a module can take in multiple exams at the same time.

If this is only for GeneralEvent, is it better to have AddGeneralEventCommand instead to be more specific and clear

Perhaps can add example of the exact delete exam command

maybe editIndex can be a better attribute name in this context.

Possible assertions can be put here to detect null values after assigning.

Update CI status, codecov, sitemap and acknowledgement

Fixes #61 #62

solves #66 #111

Shouldn't this be under 1a? Since you type the index in step 1.

Shouldn't there be a MSS for this where you state that you request for the list of events to be set to the current date?

Consequently you can put this under extensions 1a 😃

Can just use getFilteredEventList().size() + 1

Are all these commented out test supposed to be added back in later?

Is the else supposed to return a cross instead of empty string? If not maybe change the javadoc accordingly.

This is pretty inefficient I guess, can change to utilise a Set.

Set>Event> set = new HashSet>>(events);

return set.size() == events.size();

Alternatively (slightly faster time):

Set>Event> set = new HashSet>>();

for (Event e: events)

if (!set.add(e)) return false;

return true;

Ok!

Alright, I agree with the standardisation requirement. However, since the purpose of this PR is to implement Events, I think it is out of scope to change the UniquePersonList as well to maintain standardisation. We can start a new issue for this.

Added in latest commit, multiple tags shouldn't be allowed, but can start an issue to add that functionality.

This one I not sure but I think because the purpose is to list all tags then tags is better?

Adopted in latest commit.

Changed to "noNameAndNoTag" in latest commit.

Adopted in latest commit.

Adopted suggestion!

LGTM

Will implement/fix find, list, tags methods

LGTM

But when invalid, do we want it to throw error? or isit better to just do nothing

I think we should throw an error so the user understands what is wrong if they try to use the feature wrongly.

Good use of assert statement here!

Plural form here helps to identify the multiple values available, good job!

There's a redundant new line here, can remove if not required.

Good use of for-each loop, its very readable!

Duplicate Issue that has already been created.

Task Complete.

Completed Task.

Feature already implemented in AddressBook.

Already implemented in AddressBook.

Completed.

Tutorial, not required to merge.

Sorry, i pull requested to the wrong team project.

Sochedule?

No need to check null?

Maybe consider YYYY-MM-DD (all upper, instead of partially upper), then hh:mm (all lower)

Should this be removed or it's waiting for model dependencies removal?

Thanks for spotting the error:)

Yeah I think Deadline should under Task package. LGTM:)

Can these changes can pass all the current unit tests?

It didn't pass, since the unit tests now are mostly old ones that haven't been updated yet.

Need to enable four tests in AddTaskCommandParserTest and AddEventCommandParserTest

Should "Appointment's %s field is missing!" be "Property's %s field is missing!"?

Should update the comment to properties

Should empty Appointment be empty AppointmentBook?

Should update address book to property book?

Updated already

I think it should be one command for both property book and address book? Which one is better?

Ok, I got what you mean. So split the command into 3

Ahhaha, this one is not implemented yet

Not implemented in its class. I will try to add it later

Thanks, will update it

Ok, will update it

Fixed

It is strange that it is displayed as "9:00am" on my laptop. maybe due to java versions?

Restored

Forgot to mention, I changed the sorting order to be compulsory for easier implementation

actually %1$s is replaced by the string "asc" or "desc"

The test for SortAppointmentCommand is not done yet. The current content is simply copied from another file

Yes, so do not merge until I finish the rest

Should be able to merge now. Finished adding tests

The method actually sorts the appointment list but the displayed filtered appointment list is sorted accordingly. Should I change back the javadoc comment?

updated

Fixed. Thanks!

Fixed

Actually, in AppointmentBook::resetData, only the appointmentList gets replaced

I have added getCommandWord(String) in PocketEstateParser

can

fixed

Thanks for your kind suggestion.

Sure!

Only supports undo for add, delete, edit commands. Tests will be added later.

I like how you explained the implementation of Schedule.

Looks good.

Just this > **Examples** has an extra space compared to every other ones in markdown, but visual wise does not change anything

Perhaps Deleting a passenger would be more consistent

If I'm not wrong, the regex doesn't validate whether the number given is a negative number or not right?

maybe can do ^[1-9] at start of the pattern to make sure there's no negative sign I guess?

Can also test for negative price as those should be invalid too!

LGTM, all links added are correct also

LGTM

LGTM also

LGTM

It should be optional for radd so can you update it?

Ah, I was referring to adding the brackets i.e. [r/ROOM] for optional tag

missing javadocs?

Done!

Good catch!

Will do in later iteration

Done!

Good catch!

Done!

LGTM

is this a typo?

mechanism*

should it be feature instead of mechanism?

Yup sorry slipped my mind our project name changed.

yup sorry for the typo!

Yup! i will change it.

Need to fix checkstyle error

We copy this from the original implementation of AddressBook

Agree, to be consistent with the other prefixes in the CliSyntax.java, I think w/, h/ and mc/ is better

ListCommandCommand? How about ListOfCommandsCommand?

merged already

This can be shortened to just this.zeroBasedIndex - index.zeroBasedIndex. (I think)

While it may already be intuitive, maybe you can add a comment that the function sorts the dates according to the earliest to the latest.

I am not too sure what the variable value means here.

fixed

yep, changed the conditions to be more strict.

oops, meant to be RemindCommand

changed

Looks great!!

Also, wanted to ask whether inputting an add/find command after a remind command changes the address book back to the original person list? Not sure if the addressbook already takes care of that somewhere else.

@pavz02 , RemindCommand acts on a different list than add command, but on the same "filtered" order list as FindCommand, it will not affect the state of the original person list.

it should be initialized as an empty string, thanks for catching that, i forgot to test the addcommand

changed

added more requests

i was wondering why the ab3 tutorial added a prefix too, i think its used to mark the start of the request, but im not too sure why they added it in the first place, so i left it as it is.

yep, for now it looks like the tag and undelivered tag, wanted to make it stand out more.

changed

LGTM

LGTM, maybe you can update the user guide as well to reflect the changes that have been made.

LGTM, will be merging this to the master branch so that I can fix some merge conflicts on my end.

Fix #85

Skimmed through it and left some comments you might want to consider. Other than that, LGTM 👍

edit: also I'm guessing you're not planning to add requests using the Add command?

I am still unsure of how an order requires a request but the request is not added with the add command. I thought you would be implementing it with the add command as an optional field (optional like the tags), so if I misunderstood your implementation sorry haha

It is implemented with a separate command, but now that i think about it i should have probably implemented it with the add command lol i thought it would be neater to use a separate command to add requests instead of overloading the addcommand. In my implementation, requests are initialized to be empty at first and added with a separate command.

LGTM

I believe that Model actually has a hasEvent(int index) method that can be used here instead!

Is there a need to change the boolean form here?

Very clear documentation!

thanks for pointing that out!

Ah thats a good idea!

thanks!

yes marcus pointed it out too. I fixed it alr!

hmm the null is okay because we check again in EditAssignmentCommand.java

i can do the event skeleton, add and edit

Minor spelling mistake here.

Should be matched instead of matches.

Would it be better if 60 is declared as a constant?

Can I ask what's the consideration for changing .trim() to .stripLeading()?

@oeiyiping I have actually pondered over this for some time, did some research and decided to use the same class name.

As mentioned by @justgnohUG, since they are clearly differentiable by file path, and on top of that, there is an extremely low possibility that you will be referencing both Command classes in another class, I don't see any issues.

Feel free to share your thoughts and opinions again.

For this, I followed the original AB3 boolean variable name showHelp and therefore named the boolean variable showAlias.

How about changing both to shouldShowHelp and shouldShowAlias?

I agree with what you have said.

Will update it, push it into the branch and request for your review again later!

Update this code.

There is currently no sample or default aliases data, that is why a new UniqueAliasMap() is initialised here instead.

Added Javadoc for the aliases param.

Updated this code in handleAlias and similarly for handleHelp.

Updated this code.

Updated this code and added missing Javadocs for both parseTagsExceptLast and parseTags.

Will add this once I merge PR #77 into my branch.

Added Javadocs for this unchecked exception.

Would it be okay to refactor this in Milestone 1.3? I will raise an issue after this PR is merged.

Updated the boolean variable to shouldReturnAlias.

LGTM

In ModelManager, should line 107

updateFilteredPersonList(PREDICATE_SHOW_ALL_PERSONS); of

public void addPerson(Person person) method

be updated/removed to ensure the filter is persistent even after adding a person?

Other than this, everything else LGTM for Logic component.

This line should not affect the filter after adding a new person

Alright, LGTM.

Conventions are not compulsory to be added for DG and will be tracked and discussed offline. Closing issue.

Implemented in PR #76.

After testing the new prefix parsing implemented in this PR, I think there are fundamentally a few problems here, especially with the filter and alias command.

The filter command is having trouble working as intended. For example, if I want to filter by address, I would need to input filter -a with whitespace behind.
Similar, typing filter should clear all existing filters. However, without trimming the trailing whitespaces, the filter command will recognise the command slightly different and instead filter the list of persons to show just the name.
Alias commands are also having problems when parsing and checking the validity of the commands to be aliased.

LGTM

All done, closing issue.

is it just me or is this repeated

here

A Japanese character/word

Can change the system from addressbook to planit?

Should this be start time?

Using {:toc} allows us to see all the headers so is that better?

Alright thanks!

It is in ongoing works.

Hey can you remove the role part, since for now I think we are randomly distributing tasks so that we all get to learn equally? We discussed this yesterday i guess. otherwise LGTM!

Hey, I actually changed the names from Car@Tinder to Car@leads as you told yesterday, can you reflect these changes?

LGTM

@nighoggDatatype Need to merge the conflicts! After that will accept

Yep, LGTM

there is a large chunk of spaces(?) here, not sure if it is intended

There is a space here, not sure if it is intended?

Thanks, done!

Good job in adding both valid and invalid prices

Good idea to check all the different price ranges. Perhaps consider checking for the case when the price is MAX_INT and MIN_INT as well?

Merge Gokul About us

Good explanation. I was thinking if we could add a line mentioning that this would be more most apt for situations where the review could be too long => using the view feature shows more of the review.

Line 39: ... e.g. the review element may contain a long review that is shortened in the main window; using view will be apt for the user to view the whole review.

Centralised Ui image in Readme

Internal: Change codes to fit The Food Diary (ie. no "Persons")

I'll fix the checkstyles

will update to delete customer base on name

If we were to set Person as immutable, setDates, setMeetings and setPicture might need to shift to a different class instead. Maybe a PersonFactory?

Either this or state in comments there are mutable fields present in Person.

Add javadocs comment here.

Sounds good. Added the glossary back in.

Fixed

Test cases seems to make it hard for you to pass in a LocalDate as an argument. I changed the validation to use #41 dateUtil. Should we still make it as a LocalDate field?

The rest of the other 'with' methods doesn't do their parsing in this testutil though. I think what I'm going to do is,

Allow birthday to accept a string and parse the string. This is primarily for testing.
Create an overloaded constructor which directly accepts a LocalDate. This is what the app will be using.

Tried using stream but parseIndex throws an exception. Apparently unhandled exception does poorly with streams unless I explicitly throw it inside the lambda.

Something seems missing here?

Very minor suggestion but you can consider renaming the method to updateDeliveryStatusToDelivered just to make the name more descriptive.

I followed the same convention as the Person model where they use value to store the value of the object.

Oh yeah good point about the parser! I will edit the regex accordingly.

I think the regex in the Name class does not accept single characters.

oh yes didn't think about that! Will update it accordingly!

I am not really sure whether there are any use cases for equals. Probably will leave it in for now in case it is needed and remove it later if it is not used.

oh yeah that's a good point, will do that

Fix #12

Fix #5

Fix #14

Apart from the replacement of person by order as already mentioned by @xiinweii98, LGTM as of now.

Fix #78

It might be needed as we should allow the user to add themselves to the system regardless of whether they have been assigned a driver or not

Is the toString method call required? if this Pr is just for renaming was removing the call a mistake?

Thanks for the comment! To clarify, the VALID_[VARIABLE] constants are not used as replacements for user input but rather for the personbuilder and other such utility functions during testing. As for tests concerning user inputs, the constants declared below of the form [VARIABLE]_DESC_BOB, and as they are supposed to emulate user inputs, they are of the String datatype. Hope this addresses your comment!

Thanks for the comment! I will make the relevant changes.

Oh nice catch! I'll make the required changes!

Thanks for bringing this up. I think it might be useful as it would prevent the violation of the Law of Demeter when other classes try to call for the TripTime strings.

Good point. I removed it as Intellij suggested doing so as double data types aren't nullable but I'll add it in just in case.

Nice catch! I didn't think of it but I realised when testing that the \d+ already catches negative cases as "-" is not a digit and so the input string would fail the isValidPrice check!

You're right. I was intending to throw the same exception that the requireAllNotNull() check in the line above it does but an illegal argument exception would be more fitting.

Noted thanks for the heads up.

Resolve issue #10

It seems more appropriate to change it to StudentBook.

Perhaps you can consider placing a quotation for the command, for i.e. 'Add Student' to enhance readability?

resolve #113

Would be good to include more assertions

This might be violating Law of Demeter, could there be better way to abstract these type of statements?

What about creating methods to switch to learn mode inside the model manager?

Edited, thanks.

I changed it to isQuestion, hopefully this is better!

Ok!

same like before, would it be better to put expectedModel as a class variable?

For expectedModel, would it be better to declare it as a class instance variable instead?

Yes, you are right!

Sure

Ok!

Ok sure!

Ok!

Ok sure!

Sure!

Looks good to me. SLAP is adhered to and method is not too long or overly nested

is RemindedParser a typo?

I can do general event done and delete

GE attributes, String description, LocalDateTime

general event description prefix - g/

general event LDT prefix - on/

looks good. Follows the same format as the other edit commands and parsers

The GUI similar to the picture below

lgtm. just want to clarify the JSONArray jsonTags passed as a parameter here is a json array of all of the 'tagged' field values in our database flashcards.json?

I think so too. However only the modelmanager is passed into execute and since the mode resides in modelmanager I cant find a way to abstract it out

good idea, implemented

added to gitignore in new PR

Added greetings message for the app.

Figured out logic behind GUI display

Added greetings message for the app.

Perhaps create a PR for it, as it will probably be something useful for the application? 😃

Sure yucheng, created the PR

@xinweit after I merged my previous PR into the code base there seems to be a conflict. The conflict is in MainWindow.java and it seems its relatively simple to resolve from your side. Its a bit troublesome from my side as I would need to make changes of your PR locally, so can help?

You can resolve it by trying to pull the latest master branch to your GUI-display branch. Thx!! 😃

Sure bro ill do that, no prob

updated

right now to change the mode of display we are hiding and showing the flashcard panel using setVisibility(boolean) method of javaFX. but I think this mode class will come into handy next time when we want to say parse different commands based on the current type of mode (quiz or learn), ie. commands specific to quiz or learn mode

nice. lgtm

Should only be able to start when user is in quiz mode I think. Working on it.

Err so this PR is a super set of your PR 74 and 76? Sorry I am a bit confused

Yeah it includes those as well.

Will be closing this pull request due to a problem in pushing to this quiz-duration branch.

Should be fine I guess.

Okay will specify that it is getter for the fields. But this is still necessary because we want to keep it hidden from the rest of the classes.

But note that these changes are essential for us to have the objects be testable. Or else, you can't instantiate a ID with some particular fields without changing the underlying counter. Previously, unlike CustomerId; OrderId and CheeseId are instantiated with native constructor, with no way of not incrementing the underlying counter.

By the way, the comment was carried over from seedu.

Yeap this is updated.

Yeap this is updated in the latest push.

2F + Mockup - Wei Xue
2F + DG - Lauren
2F + UG - Li Quan
2F + README - Aaron
3F - Daniel

@daniellau88 it's your call, the workflow build fails because it tries to access the secrets from the master repository. Other than the access issues, it should be working.

Yeap seems like I have included the wrong token. I have updated the secret with the chat ID of our Telegram group.

Actually do you want to squash some of your commits, before merging. But other than that, everything seems fine.

Now the issue is the workflow works for push events, but not from the pr from forked, because GitHub doesn't expose secrets to forked-triggered workflows. Seems like we have to close this one down, as our current workflow, we are not pushing/pr-ing from master repository's branches, the notifier doesn't work in this case.

Since this will not work for our current workflow, unless using plain text tokens, I will close this down. Maybe use it for v1.3.

I will recommend squashing your commits before merging if possible.

There is one class of ModelStub with no useful methods, and the rest of variations are extending from it and overiding methods important for testing. We could make it abstract though. What else do you think we can do. The idea is we have two kind of test suites: 1 with using the actual ModelManager and 1 using just the stubs.

Fixed the above mentioned issues but also, the Json parsing to model for cheese is also not fixed with parsing null fields, and thus have some initialization issues. Have fixed all.

Maybe can change the parseTags here to a parseCategories? 🤔

Ok thanks!

Maybe i will hand over this part to you haha.

I haven't inspect on that deeper so I am not quite sure 🤔

LGTM

UserGuide still seems to reflect "iclose INDEX"? Maybe userguide can be edited to reflect the change

Yes, its optional. I showed two variations in the use examples. But I'll explicitly put that r/ is optional under the radd explanation

removed this chunk of code. Thank for pointing out.

Noted. Added actions to represent the checks. And used guard clauses.

Ok. Added the example command to the userGuide.

Added this point. Thank you.

Nice catch!! 👍

LGTM!

im so sorry i clicked the wrong link!! thank you!!

would this cause a problem where name will be searched when user searched for a student's school

execute method looks good but might want to do abit more abstractions to make the method LOC to be less than 30!

Thanks for styling the diet plans nicely! +1.

PR contributes to #18

This PR contributes to #16.

Contributes to #18 .

#18

Closing, all updated.

Should the link be from our repo instead?

I feel the command should be rephrased to foodintakeupdate for consistency with the rest

LGTM with more detailed descriptions.

lgtm.

looks good!

Perhaps the magic numbers can be replaced with named constants to make the formula easier to understand 😃

Good point! Will update this.

Updated command name.

You are right! Thanks for pointing that out!

Looks good to merge!

Looks good to merge.

LGTM

#90

I think can just leave it in the old form as ParserUtil is static but looks fine

I changed the checkstyle to allow wildcard imports since we import considerably from one package. Wildcard imports should be ok if you import more than 3 functions/constants from a package

LGTM

Nice job. Will merge when everyone has updated UG and DG

Another one

Just to have a user story to close after we finish adding stuff for v1.2

Indentation for wrapped lines should be 8 spaces according to CS2103T's coding standard. Not a major issue.

not too important but probably can import the static final variable MODE_QUIZ and MODE_LEARN so that the code looks nicer? 😄 same for the rest of the magic numbers

Fixed!

Oh thanks for catching that haha

gots it

new commit for added abstraction as well as clearer names for the image variables

Solved in #60

Typo on this line

Done

Both the BudgetDisplay() and updateObservableList() methods appear to have a number of lines of repeated code which violates the DRY principle. You could perhaps refactor your code to patch this up 😃

implement find-fr command

Would it be better if the regular expression was defined as a final variable instead?

I see! Thanks for the suggestion!

There are issues when calling progress while no active diet plan is selected as well as printing the total adherence to plan

The one in address/logic/commands is an {abstract} class and exists in a different package from this Command.java.

I think since they are clearly differentiable by file path, it shouldn't be an issue. Because alias/Command.java is named as such logically.

"Paths (or URLs) are nice because they are nested identifiers."

Reference: https://softwareengineering.stackexchange.com/questions/250481/is-it-a-bad-practice-to-give-two-very-different-files-with-the-same-general-purp

Made changes to the statement

Fixed and removed.

This test was originally to ensure the commands are sorted. Under the assumption the "add" command will always be present, regardless, the first command at index 0 should always start with a.

Perhaps a suggestion is to test every first letter of each command and see if they are alphabetical?

Modified test case following 1.

Decided to compare element by element.

Thank you for the suggestion.

I get your drift.

It was my mistake to include the Collections.sort() again right after calling the method. It is already called in getAutocompleteCommand().

I have implemented your suggestion 2 as per your previous comment. I am using the comparator method so the hard-coding of commands is minimized.

Thank you for the spot.

I've added a test case to handle Empty spaces / white spaces.

Documentation for the method getAutocompleteCommands has been added.

Fixed! Thanks

Fixed!

I believe it was accidentally added

LGTM

@yaowei-soc fixed PR naming convention to provide more context and information.

Thanks for the reference.

Bug found, commands do not show up on start-up sometimes.

Nice. Thanks for enabling assertions.

Works for me.

LGTM.

Just a thought, is it a good idea to return past month's statistics as a String? Maybe it would be a better idea to return as a List or HashMap? That way, it will be easier to format and display on the Ui side.

Good catch. Thanks!!

Is this similar to issue #74 ?

@eksinyue I don't think it will fully resolve #73. If I'm not wrong, after adding/deleting a financial record, the budget will still retain at its original displayed value. But apart from that, I think LGTM!

oh okay!! Sorry I misunderstood issue #73. I will merge this PR first without closing the issue.

Probably need to change this class name eventually to accommodate searching by tag

Perhaps this 'Implementation' section can be shifted up so that it's in a new section rather than under 'Appendix: Requirements'?

I think you must make a new class called EntryTagContainsPredicate which implements Predicate>Entry> and do your implementation for filtering tags there, then pass an instance of that class as a parameter into the updateFilteredEntryList method. The ListEntryFormatPredicate is used for listing entries only.

I like how detailed and comprehensive this is!

As a busy CS student, I want to know which deadlines are coming up, so I can focus on them first.
As a CS student who has weekly quizzes, I can be alerted when the deadline of the quiz is coming up, so that I don't miss any quiz.
As a CS student, I can check my module deadlines and information at the same location, so that I don't have to navigate between multiple tabs.
As a CS student, I can set notes for the tasks, to record information about the task.
As a CS student, I can break a task down into several smaller tasks to better organize my tasks
As a CS student, I can view my schedule in calendar format to better plan my time.
As a first-time user, I can view a help message explaining the features so that I can orientate myself with the app
As a CS student, I can delete tasks that I no longer need to track.

lgtm

Done by maurice.

Lgtm

Go to 1.3

Completed

Added

Move to 1.3

yup can create one for the appointment, but the output message would be the same, which is "The patient index provided is invalid"

Just created one. Thanks

agree, thanks for pointing out

updated. Thnanks

Hi this method is different from Method Contains.

Method Contains returns return internalList.stream().anylMatch(toCheck::equals);

Method editContains returns ireturn internalList.stream().allMatch(toCheck::equals);

i think they are different because the editContains is different from Contains

Thank you for pointing out!

Can i keep it for now? because it separates from the functions used by add-appt.

In the future if there will be some changes, i can change it easily without breaking the code for add-appt.

In the finalised version, if they are the same, i will rename them.

It has the flow of : hasEditConflictingAppointment -> hasEditAppointment -> editContains

Otherwise the flow would be: hasConflictingAppointment -> hasAppointment -> editContains.

OK. Thanks for pointing out.

just changed, yes you are right. And i didnt create field for appointment index

you are right. Removed duplicate methods

you are right. Thanks for pointing out. I just removed duplicate methods

you are right. I removed it

thanks for pointing out. changes are completed. Will push again soon

Hi, i just added in the index for appointments

yes you are right. Changes were made. Thank you

you are right.

i get what you mean. i try to change

yup, have made the changes and pushed again. Thank u

done with my commands

done w my portion

The Ubuntu and Macos CI's seem to be failing due to test cases, so for now we can ignore these.

Not too sure why the Windows CI is failing, could attempt to address test cases first.

Hey, you currently have some merge conflicts in your PR, could you update your local master branch and resolve them.

Thanks!

Can let me know if you need help with this.

Done with User Stories

Need to update the existing bullet points in README.md to include a short summary of our product.

INCOMPLETE:

Needs the current semester and master plan functionality.

@Yihe-Harry Could you close this issue if grades can now be added.

Feel free to link your relevant PR that solves this issue.

The history command does this, dependent on modules being able to be added with a grade.

Now able to work via history command.

LGTM

Attach to issue

Closed.

Still need to do integration tests of meetings with

-StorageManager

-LogicManager

Tests have not been written, will leave to after v1.3 due to time constraints.

Redo PR

LGTM

Are you planning to add support for deletion of nodes in the future?

Well done!

Indicate different roles and responsibilities of each member

Team lead: Xuanqi

Documentation: Weiming

Testing: Xuanqi

Code Quality: Jiefeng

Deliverables and deadlines: Vanessa

Integration: Jiaying

Scheduling and tracking: Vanessa

And automatic saving

Another teammate has already been assigned this task

Note: User may create an instance of Meeting but there is no way to use the instance at this point (eg. cannot add to storage)

LGTM

Commit names could be improved. Other than that, LGTM.

done

Everyone's done with individual tasks. Closing issue.

done w my portion

Done with Use Cases, added 3 use cases. Not sure if need to add more.

Completed add/delete plans

Need to standardise user guide formatting.

This task is to add/delete plans, semesters and modules as stated in the user guide

This is for singular adding of modules (i.e. if I add 3230, I may be reminded that I have not completed CS2040), different from https://github.com/AY2021S2-CS2103-W17-1/tp/issues/17

done w my portion

CI matters resolved, matters to note for future CI issues:

Always clear checkstyle before pull requests
Ensure files use LF for line breaks instead of CLRF
Ensure there is a single line break at EOF

Test PR used with Week 7 Tutorial

duplicate issue

This issue is closed with the current semester command, which is already completed.

This is already done with 1.2, with info command.

done

As a CS student who does tasks near deadlines, I want to know which task is coming the soonest, so that I can finish it first.
As a CS student who has recurring tasks every period of time (e.g. weekly, monthly, etc), I want to add the task and set a recurring time, so that I don't need to add the same task multiple times.
As a CS student, I want to have notes for each task, so that I remember important information about the task
As a CS student, I want to sort deadlines from each module, so I can estimate how much workload needed for each module
As a CS student, I want to put multiple tasks instantly, so that I don't need to submit multiple times
As a CS student, I want my reminders to show the module code and name
As a CS student, I want to move the reminders easily, so that if the deadline change I can change the reminder easily
As a CS student, I want to know how many deadlines I have and how many deadlines I have done, so I can know whether I am lacking behind or not
As a CS student, I want to have the email of the prof or ta of the module, so that I can contact them if I have an issue with a deadline
As a CS student who just finished a holiday, I want to reset my calendar, so that I can start a new semester with new deadlines

wrong issue for current iteration

Yes, i am confused with the additional tasktracker.json. I check the UserPrefs and the default path is to that tasktracker.json instead of /data

LGTM!

True haha. Will make the changes.

Got it

Will update EditAppointmentCommand to accommodate both start and end time.

Made the appropriate changes.

Allowed for modification of timeFrom and TimeTo field in EditAppointment

Looks good.

Looks good!

LGTM

Added delete_budget utility
Fixed formatting errors
Updated User guide for budget commands

LGTM

#94

Closed to allow for new PR to be made without conflicting file.

Clarification: CRUD is an acronym for "Create, Read, Update and Delete".

LGTM

Added Delete Session Command

Added the relevant classes (deleteSessionCommandParser etc) for the new delete command

updated current test files

Added Ui view for List Persons Command

List Sessions Command added

Added Ui view for List Sessions

duplicate

by checking assigned tutor, assigned timeslot

done, ive made the pr

Hi i think i resolved the conflict

As an SoC student I can filter out non-soc and soc assignments and tasks to improve productivity
As an SoC student i can manage CS coded deadlines by itself since they are not mixed with other faculties mods
As an SoC student I can set reminders for CS coded modules to remind myself of deadlines
As an Soc student i often mix up SoC mods deadlines with other modules, reducing my productivity
As an SoC student, I want to know my CS assignments and deadlines well so i will not confuse with other mods
As an SoC student, I can focus more on my core CS modules by filtering out using the SW
As an SoC Student, I can delete, edit and add CS assignments that are often my core modules. So that I can concentrate better
As an SoC student, I want to finish the CS coded assignments first before other non-CS mods, this SW can consolidate all CS assignments

Maybe they can serve as examples of low priority tasks?

As a CS student, I want to be able to record down all my upcoming deadlines and assignments so that I have a clearer view on how to manage my time effectively.
As a CS student, I would like to sort my deadlines according to the most urgent ones first, so that I can prioritise my work better.
Being a CS students with many other commitments, I would appreciate reminders when I have a CS related deadline approaching, as I might forget some of them due to my busy schedule.
As a CS student that probably has multiple ongoing coding projects, I can tag each project with the language(s) that they are written in, so I know which languages I need to be proficient in
As a CS student, I can tag each assignment with a priority level so that I know where to focus my efforts on.
As a CS student that does work daily, I would like to be able to assign my tasks to a daily to-do list so that I can plan out my day and the work that I need to accomplish for that day.
As a CS student, I would like to be able to make quick notes on the assignments that I have noted down. This could help me get out a quick idea, or leave remarks on certain tasks that might be relevant.
As a CS student, I can mark assignments that I have finished as completed so that it lets me feel a sense of accomplishment.

Will resolve merge conflicts later

Will merge in my own tutorial since this is my part!

Ok, maybe we can discuss this later? Maybe somebody changed the path accidentally

Will fix CI failures

Merging to wrong branch

Looks like some of the tests weren't refactored from ModuleName to TaskName after the merging with my previous branch

lgtm

lgtm!

looks good 😃

Thanks for rmbering abt the CI

~From my iPhone

Due to EOF at newline issue, the CI test can't pass.

But the code should function with no error.

Finish this task.

My checkstyle has some issue, so I can't see where I am wrong in terms of style. Please help me to correct those if you can run the checkstyle, thanks!!!!

Simply add the class, further job including integrate it into the model. (Coming up soon)

oh,i don't know when i space here... just delete it

it is for checkstyle i think

good

Update the index.md file to fit TimeforWheels product

Update DoneCommand

DoneCommand "done [index]" will create a checkmark on the entry

LGTM

Target user profile, value proposition, user stories, and use cases added. Glossary and NFRs are kept the same

LGTM

updated Developer guide

Added user story in DG. Linked PR for the change

all to implement

Failed tests. Close

Hey,

yup yup, my mistake

Already amended it.

Can check it again:)

hey, I just changed to Car@leads psps

I don't think this can be merged.

the photo needs to be your name and show ur face, in low resolution at least, according to the week 7 website requirements

I have already helped to change the about us and added ur image in the pull request aboutus/shizheng

Yup looks good, so we start afresh with the UG and DG

@rajobasu

I will close this, I presume your week 6 tutorial task has been fulfilled.

Don't want to clutter the open pull requests:)

@rajobasu

I will close this, I presume your week 6 tutorial task has been fulfilled.

Don't want to clutter the open pull requests:)

DONT MERGE YET, STILL TESTING

WE ARE DONE ! address is back in!

resolved by PR #22

Add instructions to the UserGuide to teach the user how you can export your contact list to another device running Helibook by copying the data file.

Decided that this issue is no longer in line with project direction

Amended

I amended and placed quotations around Add. However I did not place quotations around Student as well because I do not want to give the false impression that that is the correct input format for the Add command.

Amended

Amended. I also changed the SchoolResidence to be "0..1" to enforce that there can only be 1 SchoolResidence if present.

Amended

Noted on both, I'll make the changes

@picasdan9 I have pushed the changes, I also added the 'x' under AddCommandParser to indicate that it is garbage collected, as you suggested earlier

Now we have to integrate Type into matching as well.

Detailed explanation. Good job!

nice LGTM

lgtm, nice \ns

Merge this

This is for list command, list all tasks the user has

alright ok just for basic instructions then

For now, we will stick to this, it will definitely be applicable and can be extended if required next time.

I have fixed my comment to missing year from the starting date.

I have fix the correctness of month in starting date

I will be refactoring the email field to recurringDate in v1.3 milestone

Remember to link the PR to the issue you have been assigned

I just help to link the pull request #61 of done command and status attribute to this issue

Looks good to merge

Reopen to link PR

Looks good to merge to fixes #9

Reopen to remove traces of AB-3

fixes #9 as well

Fixed by #28

Fixed by #23

Closing this issue because is fixed with #53

Closing this issue because is fixed with #48

closed for potential feature

LGTM!

LGTM

Ayeee! You're welcome! :DDDDD

Merge will close linked issue #54

Linked issue #76

Closed as mentioned issue proposes fix

I would propose using the format below!


{

  "foodIntakeLists": [ {

    "date" : null,

    "foodIntakes" : [ {

      "name" : "test",

      "fats" : 10.0,

      "carbos" : 10.0,

      "proteins" : 10.0,

      "date" : "0001-01-01"

    }, {

      "name" : "test",

      "fats" : 10.0,

      "carbos" : 10.0,

      "proteins" : 10.0,

      "date" : "2050-06-20"

    } ]

  } ]

}

After discussion, proposed solution:


{

  "foodIntakes" : [ {

    "name" : "test",

    "fats" : 10.0,

    "carbos" : 10.0,

    "proteins" : 10.0,

    "date" : "0001-01-01"

  }, {

    "name" : "test",

    "fats" : 10.0,

    "carbos" : 10.0,

    "proteins" : 10.0,

    "date" : "2050-06-20"

  } ]

}

For issue #77

Thanks for quick fix!!

Looks good to merge!

Thanks!

Failed CI checks

Failed CI tests

Conflicts

Failed checkstyle

Fail CI tests

Failed CI tests

Pull first then add the date field since the latest commit has passed all the CI checks.

I copy this design from the addressbook code,

Thanks Prof~

@Nanxi-Huang did u pull the latest version before you modify the file, cause right now there seems to be a conflict happening.

LGTM

Cannot pass the test

lgtm

Lgtm

Seems like my checkstyle not running in DeveloperGuide.md

@Andrewzhang217 I never add a new line at EOF in one of my puml file. It's fixed now. Thanks

Add a new line at EOF at AddReaderSequenceDiagram.puml should be able to solve the issue

Done, added a test case for invalid args

LGTM *

PR has been merged already.

Decided not to remove any fields in Person.

If add command does not specify date, default the date to 'today'

Show all tasks with one specified deadline (in 2nd column)

Show Tasks based on State

Example: 'delete completed' will delete all completed Tasks

test reopening issues

Looks good

I agree, I think the userIDcounter should be implemented perhaps in addressbook instead as certain tests fail currently due to ID not syncing up

Thanks for pointing this out! Will amend the code now!

This will take in dd/mm/yyyy, dd-mm-yyyy or dd.mm.yyyy and helps validate leap year as well. Do you want me to modify it to only support dd/mm/yyyy?

Just male and female

Alright i'll edit this. Does this mean that we input the ID of the owner when creating a Dog?

I agree, will amend this!

Is a set better? Because there is no java implementation of ArraySet

Modified it.

I agree, will change it!

I see, ok will remove these lines!

Will create new issue to add tests.

I think you can proceed to do the adding and deleting, just that the execution part don't call on model to do the adding, perhaps give a successful response and make use of the gui to get some feedback.

Alright, this is so I can test the classes I just created right?

Added DeleteActivityDiagram

I agree with your observation, though instead of removing the menu bar outright, I feel that we can replace it with a banner saying History and Response, much like your original UI mockup. We can discuss this tomorrow along with other UI tweaks we can make this coming week.

Edited

I think for this I didn't use the asObservableList method

Removed

Edited

No longer relevant.

Refactor UniqueEntityList to remove unnecessary methods that use Entity parameters.

okay i will add that!

oh, so we will include finance-related stuff also? okay no prob i will remove number 7!

LGTM

lgtm

Tutorial no need to merge thx!

missed check

Fix #26

Completed and approved

LGTM

This is for the editTask command to be added later so that the user can specify which attribute of the task to edit

I thought it was simpler to follow what was already implemented so that it is easier to check whether the description is valid too

Implemented and merged

Merge add deadline

I was thinking about that too, but when they used step 1, step 2... it was to describe the actions a user performs. Mine is more of the sequence of how the code handles the scenario, so I was not sure if it will be confusing if I use the same convention.

As an individual operating a private book loaning service. I can rank least borrowing books, so that I know customers’ preference and reduce importing of those types of books

Looks good to me

Please fix the checkstyle error before merging to master branch

This pull request looks good to me and can be merged to the master branch of our team repository

ohhh okay can i will make the changes

okay!

ok!

ohh i think i pulled this before u merged ur pr... so i dont have the task status class right now.. what shld i do?

oh its okay i got it!

done

Should we keep the rest other parts of the DG or delete them since it pertains to AB3?

I think we can just leave it in the meantime and remove it progressively in the future.

Some tests are failing, try running tests locally to see whats happening

Yep, I have fixed the error

would be good to test if the list of tasks is sorted after reopening the app

I think it is. However, you will need to use the exit command so that the storage saved the latest updated task list

LGTM, the command execution only updates the currently active tab right?

Erm I am trying to remodel it such that Users can only edit on the home page. The other tabs are more like viewing the task (completed, expired) etc

Error in commiting, please do not merge

Error, please ignore

good edit.

lgtm

looks good to be merged!

LGTM 👍

LGTM!

lgtm

Good job adding the methods required for borrowing books to readers! Maybe we might want to consider separating/abstracting the methods related to borrowing/returning books in the SmartLib class to a separate class?

There were some bugs with my local tp file, this pull request is just to check whether my local tp file has been successfully debugged.

Will merge this first, so that the others who are trying to update the developer guide will not have to perform these edits again.

This pull request looks good to be merged into our group's repository!

This pull request looks good to be merged into our team's repository! 👍

my bad i put ur link in and it just gave me our team's one so i thought it was the same oops 😅

yea i agree that the INDEX_ references are not that necessary

ok i'll look into it another time! let's merge this PR first and i ll continue from there onwards 😃

I am merging this PR here, since integrating the Date class with the Person class would mean a lot of work. So, better to keep this part of the work intact, and do the remaining part by generating a new PR.

Hi, could anyone tell me why I am failing the test cases?

Okay, I passed all test cases and the app runs fine with all the features working after adding Date.

Yes, I was just thinking about that too. Looking at the other methods that were implemented in BudgetBabyLogicManager, I realised that it shouldn't be a problem if I returned a List. Thanks for the suggestion!

I think it would be good if you could make this title more specific? Like what aspects of a month in particular? As @eksinyue mentioned, I also feel this PR is similar to #74, solely from the title.

I just wanted to leave the hash-tag before the line.

Yea, you can refer to our submitted user guide.

Mostly formatting issues as far as I can tell, you can just amend your commits and force push to this branch, no need to close and create another pull request. Or maybe you want to do the formatting once after everything is in?

I closed it because idk how to rename the commit message xD.

Ok, I will remove it.

Yea, I know but the codestyle forces me to do so. Idk why lol

I noticed that too

Ah, I see! Coz I was trying to make it to look the same with the MESSAGE_UNKNOWN_ENTITY and MESSAGE_UNKNOWN_COMMAND that were used in addressbook.

Noted!

Yea, I made it public just for the testing. Does that mean I only have to test public methods that are written?

Weird, didn't notice this earlier on. What can I do if something like this pops out?

OK, I will check that again. Coz it shows an error of separator wrap which is regarding the brackets.

Made the necessary changes.

I think we can finalize the formatting in the meeting later. Maybe when we merge, we can just put the parts that others have edited into this.

Noted! Sorry for the mails.

Sorry, I don't really get you. Can you tell me what's wrong and necessary? I thought I tested on the changes made in your PR?

Sync master

Ignore this! This is duplicated!

I will upload the test cases in another PR when this edit command works yet.

tP comments dashboard

1. TAN JUN WEI @w2vgd (100 comments)

2. VEVEK SELVAM @vevek (99 comments)

3. KOU YONG KANG @kouyk (86 comments)

4. FANG JUNWEI, SAMUEL @samuelfangjw (50 comments)

5. CHEN YAN JIA, JAY @JayChenYJ (48 comments)

6. DANIEL LAU YEW EN @daniellau88 (46 comments)

7. TAN JUN HAN IVAN @ivantjh (44 comments)

8. HO WEI JUN SHERMAN @nighoggDatatype (42 comments)

9. ONN WEI SIN @onnwards (41 comments)

10. LOW EN HAO @enhao25 (39 comments)

11. SOH XIN WEI @xiinweii98 (37 comments)

12. KHOO MING WEI, BENEDICT @benedictkhoomw (33 comments)

13. SIDDIQUI UMER HASAN @umergta (32 comments)

14. NGUYEN DUC DANH @Eriksen2411 (31 comments)

15. WONG KOK IAN @wongkokian (30 comments)

16. MOHAMED FAZIL S/O WAHID ALI @Md-Fazil (29 comments)

17. GARY LEE JIA JIN @garyljj (28 comments)

18. LI RUOCHEN @lirc572 (28 comments)

19. LEE XUAN WEI, JEREMY @jlxw48 (26 comments)

20. HOW YU HIN CHESTER @chesterhow (25 comments)

21. TAN JIN @tjtanjin (25 comments)

22. SWA YONG SHEN @swayongshen (25 comments)

23. NG ZHI CAI @nhzaci (25 comments)

24. NICHOLAS TANVIS @nicholastanvis (25 comments)

25. PEH YU XIANG @pyuxiang (24 comments)

Non-functional requirements

@.**** commented on this pull request.

26. LEE WEI YI, SAMUEL @samleewy (24 comments)

27. NG WEI XUE @weixue123 (23 comments)

28. CHONG YAO WEI @yaowei-soc (23 comments)

29. WONG CHOON WEI @yungweezy (23 comments)

30. YEOH HSIN YING CANDICE @candyhy (21 comments)

31. DAVID AU WEI EN @dvdweien (21 comments)

32. JASON TAY @jay9645 (20 comments)

33. GOH JUN WEI @JonahhGohh (20 comments)

34. KUMARAN S/O SELVVARATNAM @kumsssss (19 comments)

35. HE QIYUAN @QY-H00 (19 comments)

36. TAY YI HSUEN @yhtMinceraft1010X (19 comments)

37. DYLAN GHEE SHAO KANG @zatkiller (18 comments)

38. LEE PAI JOO, YVELLE @Ellevy (16 comments)

39. CHEW HONG WEI TERENCE @chewterence (16 comments)

40. PRINCESS PRISCILLA PAULSON @pPris (16 comments)

41. TEO HOE KEAT @TeoHoeKeat (15 comments)

42. LEE YUE YANG @lyueyang (15 comments)

43. ALZAHRANI RIYADH HAMDAN S @riyadh-h (14 comments)

44. CHEW WEI HAO @chewwh09 (14 comments)

45. OEI YI PING @oeiyiping (13 comments)

46. MAX NG KAI SHEN @maxxng (13 comments)

47. JULIET TEOH QIAN YING @JulietTeoh (13 comments)

48. FOO CHUAN LE, NICHOLAS @nickyfoo (12 comments)

49. YAP JING KANG @SoulUseless (12 comments)

50. YU JIAXIANG @litone01 (10 comments)

51. WANG YUANZHE @AiwassPrime (10 comments)

52. CLARISSA LEE ZI XIN @clarlzx (10 comments)

53. AMANDA ANG YEE MIN @github-amanda (10 comments)

54. PANG JIARONG, JACOB @Jacob-Pang (9 comments)

55. AZEEM ARSHAD VASANWALA @DrWala (9 comments)

56. DING YOU JIA, DANELYNN @icelenaugust (9 comments)

57. SUN YUCHENG @Cheng20010201 (9 comments)

58. THYAGARAJAN PAVITRA @pavz02 (9 comments)

59. LIU YONGLIANG @tlylt (9 comments)

Codecov Report

master #327 +/-

60. ONG JUN XIONG @ong6 (9 comments)

61. LIM LI QUAN @AhQuanz (8 comments)

62. JOEL HO ENG KIAT @JoelHo (8 comments)

63. ARIHANT JAIN @arihantjain97 (8 comments)

64. JOVEN PUA ZAI XIONG @CSmortal (8 comments)

65. LEE ZE XIN @figo2127 (8 comments)

66. SUNAGA SHION @nowknowing (8 comments)

67. PNG ZHENG JIE, SEBASTIAN @pngsebastian (7 comments)

68. LIN LUE @lue97 (7 comments)

69. TAN KENG IUAN @DarkDestry-t (7 comments)

70. VU MINH HIEU @vuminhhieunus2019 (7 comments)

71. KINGSLEY KUAN JUN HAO @kingsleykuan (6 comments)

72. ONG JING KEAT, MARCUS @markuz5116 (6 comments)

73. LOO YEOU TZER @yeoutzer (6 comments)

74. NOEL MATHEW ISAAC @noelmathewisaac (6 comments)

75. TRAN NGUYEN MANH DAN @picasdan9 (6 comments)

1. TAN JUN WEI `@w2vgd` (100 comments)

2. VEVEK SELVAM `@vevek` (99 comments)

3. KOU YONG KANG `@kouyk` (86 comments)

4. FANG JUNWEI, SAMUEL `@samuelfangjw` (50 comments)

5. CHEN YAN JIA, JAY `@JayChenYJ` (48 comments)

6. DANIEL LAU YEW EN `@daniellau88` (46 comments)

7. TAN JUN HAN IVAN `@ivantjh` (44 comments)

8. HO WEI JUN SHERMAN `@nighoggDatatype` (42 comments)

9. ONN WEI SIN `@onnwards` (41 comments)

10. LOW EN HAO `@enhao25` (39 comments)

11. SOH XIN WEI `@xiinweii98` (37 comments)

12. KHOO MING WEI, BENEDICT `@benedictkhoomw` (33 comments)

13. SIDDIQUI UMER HASAN `@umergta` (32 comments)

14. NGUYEN DUC DANH `@Eriksen2411` (31 comments)

15. WONG KOK IAN `@wongkokian` (30 comments)

16. MOHAMED FAZIL S/O WAHID ALI `@Md-Fazil` (29 comments)

17. GARY LEE JIA JIN `@garyljj` (28 comments)

18. LI RUOCHEN `@lirc572` (28 comments)

19. LEE XUAN WEI, JEREMY `@jlxw48` (26 comments)

20. HOW YU HIN CHESTER `@chesterhow` (25 comments)

21. TAN JIN `@tjtanjin` (25 comments)

22. SWA YONG SHEN `@swayongshen` (25 comments)

23. NG ZHI CAI `@nhzaci` (25 comments)

24. NICHOLAS TANVIS `@nicholastanvis` (25 comments)

25. PEH YU XIANG `@pyuxiang` (24 comments)

26. LEE WEI YI, SAMUEL `@samleewy` (24 comments)

27. NG WEI XUE `@weixue123` (23 comments)

28. CHONG YAO WEI `@yaowei-soc` (23 comments)

29. WONG CHOON WEI `@yungweezy` (23 comments)

30. YEOH HSIN YING CANDICE `@candyhy` (21 comments)

31. DAVID AU WEI EN `@dvdweien` (21 comments)

32. JASON TAY `@jay9645` (20 comments)

33. GOH JUN WEI `@JonahhGohh` (20 comments)

34. KUMARAN S/O SELVVARATNAM `@kumsssss` (19 comments)

35. HE QIYUAN `@QY-H00` (19 comments)

36. TAY YI HSUEN `@yhtMinceraft1010X` (19 comments)

37. DYLAN GHEE SHAO KANG `@zatkiller` (18 comments)

38. LEE PAI JOO, YVELLE `@Ellevy` (16 comments)

39. CHEW HONG WEI TERENCE `@chewterence` (16 comments)

40. PRINCESS PRISCILLA PAULSON `@pPris` (16 comments)

41. TEO HOE KEAT `@TeoHoeKeat` (15 comments)

42. LEE YUE YANG `@lyueyang` (15 comments)

43. ALZAHRANI RIYADH HAMDAN S `@riyadh-h` (14 comments)

44. CHEW WEI HAO `@chewwh09` (14 comments)

45. OEI YI PING `@oeiyiping` (13 comments)

46. MAX NG KAI SHEN `@maxxng` (13 comments)

47. JULIET TEOH QIAN YING `@JulietTeoh` (13 comments)

48. FOO CHUAN LE, NICHOLAS `@nickyfoo` (12 comments)

49. YAP JING KANG `@SoulUseless` (12 comments)

50. YU JIAXIANG `@litone01` (10 comments)

51. WANG YUANZHE `@AiwassPrime` (10 comments)

52. CLARISSA LEE ZI XIN `@clarlzx` (10 comments)

53. AMANDA ANG YEE MIN `@github-amanda` (10 comments)

54. PANG JIARONG, JACOB `@Jacob-Pang` (9 comments)

55. AZEEM ARSHAD VASANWALA `@DrWala` (9 comments)

56. DING YOU JIA, DANELYNN `@icelenaugust` (9 comments)

57. SUN YUCHENG `@Cheng20010201` (9 comments)

58. THYAGARAJAN PAVITRA `@pavz02` (9 comments)

59. LIU YONGLIANG `@tlylt` (9 comments)

60. ONG JUN XIONG `@ong6` (9 comments)

61. LIM LI QUAN `@AhQuanz` (8 comments)

62. JOEL HO ENG KIAT `@JoelHo` (8 comments)

63. ARIHANT JAIN `@arihantjain97` (8 comments)

64. JOVEN PUA ZAI XIONG `@CSmortal` (8 comments)

65. LEE ZE XIN `@figo2127` (8 comments)

66. SUNAGA SHION `@nowknowing` (8 comments)

67. PNG ZHENG JIE, SEBASTIAN `@pngsebastian` (7 comments)

68. LIN LUE `@lue97` (7 comments)

69. TAN KENG IUAN `@DarkDestry-t` (7 comments)

70. VU MINH HIEU `@vuminhhieunus2019` (7 comments)

71. KINGSLEY KUAN JUN HAO `@kingsleykuan` (6 comments)

72. ONG JING KEAT, MARCUS `@markuz5116` (6 comments)

73. LOO YEOU TZER `@yeoutzer` (6 comments)

74. NOEL MATHEW ISAAC `@noelmathewisaac` (6 comments)

75. TRAN NGUYEN MANH DAN `@picasdan9` (6 comments)

76. ZOU YUTING `@yutingzou` (5 comments)

77. V R SOORYA `@VRSoorya` (5 comments)

78. ZHENG RUOXIN `@zhengruoxin` (5 comments)

79. HO JIE FENG `@hojiefeng` (5 comments)

80. SOO QI XUAN IAN `@stein414` (5 comments)